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

More RDoc for Dir.glob #8088

Merged
merged 7 commits into from Jul 20, 2023
Merged

More RDoc for Dir.glob #8088

merged 7 commits into from Jul 20, 2023

Conversation

BurdetteLamar
Copy link
Member

New globbing doc for other files to link to (in future PRs).

@BurdetteLamar BurdetteLamar added the Documentation Improvements to documentation. label Jul 17, 2023
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.

I took a quick look at this file, it covers a lot of topics and goes into quite a bit of details about the methods that perform globbing.

However, it's difficult for me to review because I'm not sure how it will be used. It's difficult for me to understand how much detail we should be going into and whether or not we should be talking about the globbing methods because I don't know what existing information it replaces.

It would make it easier for me to review if you could also edit the parts that this document replaces.

Comment on lines 222 to 227
The FNM_CASEFOLD flag specifies that the matching is to be case-insensitive:

Dir.glob('c*').take(5)
# => ["CONTRIBUTING.md", "COPYING", "COPYING.ja", "ccan", "class.c"]
Dir.glob('c*', flags: File::FNM_CASEFOLD).take(5)
# => ["CONTRIBUTING.md", "COPYING", "COPYING.ja", "ccan", "class.c"]
Copy link
Member

Choose a reason for hiding this comment

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

FNM_CASEFOLD is only useful for File.fnmatch, and not for any other methods (including Dir.glob). See the documentation.

Comment on lines 242 to 245
<tt>'{_a_,_b_}'</tt>, which matches pattern _a_ and pattern _b_;
behaves like
a {regexp union}[https://docs.ruby-lang.org/en/master/Regexp.html#method-c-union]
(e.g., <tt>'(?:_a_|_b_)'</tt>):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think italics work in <tt> blocks, so the underscores will be generated.

Copy link
Member Author

Choose a reason for hiding this comment

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

The entire block is misaligned (displayed as code). When that's corrected (next push), the italics work.

But the link there is fouled by <tt>'{_a_,_b_}'</tt> (as we've seen elsewhere), so we get {regexp union}[https://docs.ruby-lang.org/en/master/Regexp.html#method-c-union] in the output. If I experimentally remove that text, the link is rendered correctly.


==== Flag File::FNM_SYSCASE

Case sensitivity (sensitive or insensitive) is the same as the system.
Copy link
Member

Choose a reason for hiding this comment

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

We should note that it's also only useful for File.fnmatch and not any other methods.

@BurdetteLamar
Copy link
Member Author

BurdetteLamar commented Jul 19, 2023

I took a quick look at this file, it covers a lot of topics and goes into quite a bit of details about the methods that perform globbing.

However, it's difficult for me to review because I'm not sure how it will be used. It's difficult for me to understand how much detail we should be going into and whether or not we should be talking about the globbing methods because I don't know what existing information it replaces.

It would make it easier for me to review if you could also edit the parts that this document replaces.

I share your concern. When I started out, I was not sure where this was heading, but wanted at least to understand the implementations.

Now I'm thinking thus:

  • Document completely at Dir.glob.
  • Document completely at Pathname.glob; "See" at Pathname#glob.
  • Document completely at File.fnmatch; "See" at Pathname#fnmatch.
  • Omit doc/globbing.rdoc entirely.

Works for you?

@peterzhu2118
Copy link
Member

Yes I agree. While duplicating the doc across several location isn't the ideal solution, I think it will be the easiest to understand for the reader.

@BurdetteLamar
Copy link
Member Author

Yes I agree. While duplicating the doc across several location isn't the ideal solution, I think it will be the easiest to understand for the reader.

I'm on it.

@BurdetteLamar BurdetteLamar changed the title [DOC] New file doc/globbing.rdoc More RDoc for Dir.glob Jul 19, 2023
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.

One minor comment, otherwise looks good!

dir.rb Outdated
Comment on lines 239 to 240
# - If optional keyword argument +base+ is not given,
# the base directory whose entries are to be matched is <tt>'.'</tt>.
Copy link
Member

Choose a reason for hiding this comment

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

This should be part of the documentation rather than as a specific note for the example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed (as redundant); minor edit to the other passage discussing +base+.

dir.rb Outdated
Comment on lines 281 to 282
# examples below use the {Ruby file tree}[rdoc-ref:Dir@About+the+Examples]):
# {Ruby project file tree}[https://github.com/ruby/ruby]:
Copy link
Member

Choose a reason for hiding this comment

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

There's sort of issue here, the Ruby file tree part is duplicated twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noticing; now fixed.

dir.rb Outdated Show resolved Hide resolved
Co-authored-by: Peter Zhu <peter@peterzhu.ca>
@peterzhu2118 peterzhu2118 merged commit 210caa7 into ruby:master Jul 20, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements to documentation.
2 participants