-
Notifications
You must be signed in to change notification settings - Fork 208
feat: exists to return the type of a path
#1026
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
Conversation
sebastian-mutz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, @wassup05!
Functionality seems fine; would strongly recommend commenting the code a little more here and there to make it easier to maintain down the line by people other than those intimately familiar with the system modules.
perazz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR @wassup05, I think the implementation is almost ready. The biggest comment I have is about harmonization with the rest of the library.
We already have is_directory, so I think we should introduce similar functionality to is_link and is_file. Same thing for delete_file and remove_directory , we probably need unlink (or remove_link) and create_link?
src/stdlib_system.c
Outdated
| case S_IFREG: type = type_regular_file; break; | ||
| case S_IFDIR: type = type_directory; break; | ||
| case S_IFLNK: type = type_symlink; break; | ||
| default: type = type_unknown; break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we differentiate between a missing and an invalid object? What do other libraries do in this respect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is possible to do that, because the respective syscalls may error out, then we don't truly know the status of the path, whether it is missing or invalid, also windows only returns INVALID_FILE_ATTRIBUTES, not differentiating between some runtime error or the path not existing, that part is done by the GetLastError and I think we could also keep it that way for simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so for now, I agree to just leave it unknown and assume we cannot differentiate between an actually missing entity and a malformed path yet.
Regarding this there is one thing I wanted to discuss.. in unix all the types such as So I wonder if we should go ahead with these |
|
This is the way that feels most natural to me:
But of course I suggest further ideas about this be discussed. |
|
That seems reasonable @perazz, I have added |
sebastian-mutz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @wassup05. The reworked and new docs and addition of is_regular_file and is_symlink look good to me. Let's wait a little to see what others think.
|
LGTM @wassup05 @sebastian-mutz, except I |
|
|
|
Hi @wassup05. Perhaps this consistency matters a little less than having a slightly more intuitive |
|
Another point to discuss I feel is, should these |
|
Can you highlight some specific advantages you see in using |
|
It's the same as many other functions that have been added over time, let's say The same reasoning can be applied to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending the attached comment @wassup05, thank you for this PR.
Regarding the logical functions, I agree that (separation of concerns) they should not return a state flag, but just match the underlying logic such is done already.
Think about condition "value>0"
-> true if value = 1.0, 1234.5, +Inf
-> false if value = 0.0, -123. NaN, -Inf
So it seems consistent to me that the edge cases are already considered by the boolean condition.
|
Request to add a test for |
User facing stuff added are
integerfunctionexists (path [, err])to check if a file exists and return it's type.integerparameters,type_unknown,type_regular_file,type_directory,type_symlinkwhich are the return values of theexistsfunction.To accomplish this, on unix
lstatis used and on windowsGetFileAttributesAis used.type_unknownis returnedFS_ERROR_CODE(usingstrerroron unix andFormatMessageAon windows).c_getstrerrnow takes alogicalwinapiargument to switch between these two functions (strerrorandFormatMessageA).