-
Notifications
You must be signed in to change notification settings - Fork 33
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
[DOC] Revisions for module-level doc #90
Conversation
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.
Can you include the "What's here" commit in the PR since this PR deletes the list of functions? Alternatively, could you restore the "Module Functions" section in this PR and remove it in the next PR? It's hard to review when this PR deletes the section and is re-written in the next PR.
lib/fileutils.rb
Outdated
# documentation for examples. | ||
# | ||
# There are some `low level' methods, which do not accept keyword arguments: | ||
# == Options |
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'm not sure what the purpose of showing all the possible keyword arguments here are. They're all described in the documentation of the methods.
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.
Just meant to replace an incomplete list with a complete one. Delete?
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.
Yeah, I'm in favor of deleting.
Will add to this PR. |
lib/fileutils.rb
Outdated
# - ::rm_f, ::safe_unlink: Like ::rm, but removes forcibly. | ||
# - ::rm_r: Removes entries and their descendants. | ||
# - ::rm_rf, ::rmtree: Like ::rm_r, but removes forcibly. | ||
# - ::rmdir: Removes directories and their descendants. |
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.
rmdir
does not remove descendants (only empty directories)
irb(main):001:0> FileUtils.mkdir("test")
=> ["test"]
irb(main):002:0> FileUtils.touch("test/foo")
=> ["test/foo"]
irb(main):003:0> FileUtils.rmdir("test")
/Users/peter/.rubies/ruby-3.1.2/lib/ruby/3.1.0/fileutils.rb:261:in `rmdir': Directory not empty @ dir_s_rmdir - test (Errno::ENOTEMPTY)
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.
Fixed.
lib/fileutils.rb
Outdated
# - ::collect_method: Returns the names of methods that accept a given option. | ||
# - ::commands: Returns the names of methods that accept options. | ||
# - ::have_option?: Returns whether a given method accepts a given option. | ||
# - ::options: Returns all option names. | ||
# - ::options_of: Returns the names of the options for a given method. | ||
# - ::pwd, ::getwd: Returns the path to the working directory. | ||
# - ::uptodate?: Returns whether a given entry is newer than given other entries. |
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 think everything except pwd
and uptodate?
should be moved to a different section (perhaps called "Metadata", or a better name if you can think of one) since they're information about the methods in this class.
Only pwd
here is actually related to querying file operations.
I think uptodate?
should be in the "Comparing" section since it's comparing timestamps of files.
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 disagree. I have not claimed that all these query the file system, but only that they are queries (i.e., return information, rather than making changes).
I don't think it's a stretch to view a method that returns a boolean as a query.
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 disagree. I have not claimed that all these query the file system, but only that they are queries (i.e., return information, rather than making changes).
I realize that the docs doesn't claim to query the file system, but everything else are file system related except these meta methods, so it may be a bit confusing for the reader.
Tangentially related, I don't understand the use case of these meta-querying methods. I can't see why a user has to check what keyword arguments are accepted by a method or what method accepts a particular keyword argument.
I don't think it's a stretch to view a method that returns a boolean as a query.
I think I would define a query as something that returns information about the file. For example, checking if a file exists (which returns a boolean) is a query. A comparison is taking two files and comparing their data, returning true or false. For example, checking if two files contains the same contents is a comparison. I think comparing metadata (which is what uptodate?
is doing) falls under comparison.
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 see the use case either.
I''ve named the new section "Options" even though it's not a gerund.
Can you live with ::uptodate? remaining in Querying, so ::pwd won't be lonely?
FYI, what's left to do here (in future PRs):
|
) * Revisions for module-level doc ruby/fileutils@dcbad90a1f
Next PR will have "What's Here" section. Any thoughts about the ordering of the sections?