Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Path_exists() function #1106

Merged
merged 7 commits into from
Feb 21, 2022
Merged

Path_exists() function #1106

merged 7 commits into from
Feb 21, 2022

Conversation

heavelock
Copy link
Contributor

@heavelock heavelock commented Feb 17, 2022

Hey!
So here is promised PR for path_exists().
I also added tests for dirs and files, should I also add tests for hard/soft links? And sorry for their general clumsiness, I'm writing rust basically for the first time and I still have to wrap my head around it.

Closes: #1102

heavelock and others added 4 commits February 17, 2022 08:50
Signed-off-by: Damian Kula <heavelock@gmail.com>
Signed-off-by: Damian Kula <heavelock@gmail.com>
Signed-off-by: Damian Kula <heavelock@gmail.com>
Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! So here is promised PR for path_exists(). I also added tests for dirs and files, should I also add tests for hard/soft links?

Since we're using Path::exists, which is provided to us, we don't need fully comprehensive tests. In general, I think it's safe to assume that you're dependencies are correct, especially when it comes from the standard library. So for this, I think we can just have one test when a file exists, and another when it doesn't.

And sorry for their general clumsiness, I'm writing rust basically for the first time and I still have to wrap my head around it.

No worries, rust is hard, and the way just runs tests is ideosyncratic!

tests/functions.rs Outdated Show resolved Hide resolved
tests/functions.rs Outdated Show resolved Hide resolved
tests/functions.rs Outdated Show resolved Hide resolved
tests/functions.rs Show resolved Hide resolved
Signed-off-by: Damian Kula <heavelock@gmail.com>
@heavelock
Copy link
Contributor Author

Thanks for your comments! I applied your suggestions and removed the tests for directories since we don't necessarily need to test for them.

Should I somehow refer in docs that this function is using internally rust's std behaviour?

@casey
Copy link
Owner

casey commented Feb 21, 2022

I think it probably isn't necessary to reference the stdlib function. I looked at the documentation for Path::exists, and we could probably include some of the details. I also think it's probably a good idea to put the function under a more descriptive heading, maybe, so something like:

#### Filesystem Access

- `path_exists(path)` - Returns `true` if the path points at an existing entity and `false` otherwise. Traverses symbolic links, and returns `false` if the path is inaccessible or points to a broken symlink.

Signed-off-by: Damian Kula <heavelock@gmail.com>
@heavelock
Copy link
Contributor Author

I changed the readme entry according to your suggestion.

@casey casey enabled auto-merge (squash) February 21, 2022 21:42
@casey casey merged commit 6271e94 into casey:master Feb 21, 2022
@casey
Copy link
Owner

casey commented Feb 21, 2022

Nice, merged! Thank you for the PR! I think there are probably a lot of other useful boolean functions that can be added now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

path_exists() Path manipulation function
2 participants