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

[DOC] Revisions for module-level doc #90

Merged
merged 3 commits into from
Jun 17, 2022
Merged

[DOC] Revisions for module-level doc #90

merged 3 commits into from
Jun 17, 2022

Conversation

BurdetteLamar
Copy link
Member

Next PR will have "What's Here" section. Any thoughts about the ordering of the sections?

Copy link
Member

@peterzhu2118 peterzhu2118 left a 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
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

@BurdetteLamar
Copy link
Member Author

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.

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.
Copy link
Member

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)

Copy link
Member Author

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
Comment on lines 53 to 59
# - ::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.
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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?

@BurdetteLamar
Copy link
Member Author

FYI, what's left to do here (in future PRs):

  • Add links to off-site man pages.
  • Add more Related remarks.
  • Put trees into a few examples.

@BurdetteLamar BurdetteLamar merged commit dcbad90 into ruby:master Jun 17, 2022
matzbot pushed a commit to ruby/ruby that referenced this pull request Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants