-
Notifications
You must be signed in to change notification settings - Fork 192
feat: creating and removing empty directories #1011
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
Hey @wassup05. Thanks for the PR. Great work!
- I added minor suggestions to the comments (just grammar).
- For
example_remove_directory
, I wonder if it makes sense to first create the directory. Ideally an example only demonstrates the use of one procedure, but without the existing directory, it can't. Just a thought and nothing major.
For example_remove_directory, I wonder if it makes sense to first create the directory. Ideally an example only demonstrates the use of one procedure, but without the existing directory, it can't. Just a thought and nothing major.
I think it's fine @sebastian-mutz, doing that would kind of clutter the example I feel, and this is how it's been handled in some other examples as well like here and here
For example_remove_directory, I wonder if it makes sense to first create the directory. Ideally an example only demonstrates the use of one procedure, but without the existing directory, it can't. Just a thought and nothing major.
I think it's fine @sebastian-mutz, doing that would kind of clutter the example I feel, and this is how it's been handled in some other examples as well like here and here
Makes sense. Then it's best to keep it consistent.
Concerning the mode
argument which is not applicable to Windows, here is how some other languages handle it...
Python
Complete docs here: https://docs.python.org/3/library/os.html#os.mkdir
To quote some important parts:
On some systems, mode is ignored. Where it is used, the current umask value is first masked out. If bits other than the last 9 (i.e. the last 3 digits of the octal representation of the mode) are set, their meaning is platform-dependent. On some platforms, they are ignored and you should call chmod() explicitly to set them.
On Windows, a mode of 0o700 is specifically handled to apply access control to the new directory such that only the current user and administrators have access. Other values of mode are ignored.
Golang
Golang does it in a different way by creating a portable custom struct
of the permission bits, to quote exactly
A FileMode represents a file's mode and permission bits. The bits have the same definition on all systems, so that information about files can be moved from one system to another portably. Not all bits apply to all systems. The only required bit is ModeDir for directories.
Reference: https://pkg.go.dev/os#FileMode, https://pkg.go.dev/os#Mkdir
Rust
Rust seems to have dropped this argument completely and uses default permissions
Reference: https://doc.rust-lang.org/std/fs/fn.create_dir.html
Regarding the mode
argument:
It seems like Go's and Rust's handling are more in line with the previously discussed approach of avoiding the consideration of platform specifics by stdlib users. I wonder if Go's struct-based approach could be mirrored in Fortran through derived type. Then again, it's been my experience that "keep it simple now, add complexity later (if needed)" usually creates the fewest headaches down the line. Consequently, I'm leaning towards Rust's very simple approach, but I'm curious what others think.
ac195a0
to
f27214b
Compare
f27214b
to
ab27ae0
Compare
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 the PR update, @wassup05! Looks good. I left some minor comments for more clarity and comment style consistency.
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'll have a last look at the code itself later, but looks good and works for me so far.
db9c14c
to
95e76be
Compare
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.
Hey, @wassup05. The PR LGTM as it is. Thanks!
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 @wassup05, thank you for this PR. I only have one tiny comment left before I think this is ready to merge.
In absence of further comments, and with two approvals, I will go ahead and merge this one.
User facing functions added are
make_directory (path, mode, err)
(mode
argument only for Unix systems)remove_directory (path, err)
/
or\
.CRT
functionsmkdir, rmdir
,_mkdir, _rmdir
on Unix and Windows respectively.strerror
has been included as a private helper to provide helpful error messages.state_type
is used for error handling.Side note:
make_directory
could have alogical
argumentrecursive
which would work likemkdir -p
but that would require path manipulation especiallydir_name
from #999Do let me know your thoughts.