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

Fix docs error for File.match? ** globbing pattern. #12343

Merged
merged 2 commits into from
Sep 20, 2022

Conversation

zw963
Copy link
Contributor

@zw963 zw963 commented Jul 31, 2022

This really a small changes, so, i create a PR directly.

I just copy the docs of Ruby std-lib Dir.glob to here, because it more accurately.

Thank you.

@@ -412,7 +412,8 @@ class File < IO::FileDescriptor
# * `"c*"` matches all files beginning with `c`.
# * `"*c"` matches all files ending with `c`.
# * `"*c*"` matches all files that have `c` in them (including at the beginning or end).
# * `**` matches an unlimited number of arbitrary characters including `/`.
# * `**` matches directories recursively if followed by `/`.
# If this path segment contains any other characters, it is the same as the usual `*`.
Copy link
Member

Choose a reason for hiding this comment

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

Could you give an example of what this means? I know this came from Ruby, but I still don't understand it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, Okay, maybe it should be describe better than ruby version.

let us use crystal-book project as root, i write a new 1.cr, with following content:

# 1. If we are use ** without followed by a /, or followed by others, e.g. followed by `.md`, it same as `*`

p Dir.glob("docs/**.md")
p Dir.glob("docs/*.md") # totally same as above.

# Dir.glob("docs/**.md") # => ["docs/platform_support.md", "docs/SUMMARY.md", "docs/governance.md", "docs/README.md"]
# Dir.glob("docs/*.md") # => ["docs/platform_support.md", "docs/SUMMARY.md", "docs/governance.md", "docs/README.md"]

# 2. But if ** followed by a `/`, it matches directories recursively. (behavior not same as `*`)

x =  Dir.glob("docs/**/*.md") # => ["docs/guides/ci/circleci.md", "docs/guides/ci/gh-actions.md", "docs/guides/ci/README.md",
# ...... a very long list which include all md file in any depth subdirectory ...
# "do "docs/database/connection.md", "docs/database/README.md", "docs/database/transactions.md"]
p x.size # => 143

y =  Dir.glob("docs/*/*.md") # ["docs/conventions/coding_style.md", "docs/database/README.md",
# .... a long list which include all md file in level 1 and level 2 subdirectory of docs/.
# , "docs/syntax_and_semantics/while.md", "docs/the_shards_command/README.md", "docs/using_the_compiler/README.md"]
p y.size # => 92

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

I'm thinking that **.md should actually produce a runtime error. It doesn't make sense to me if it's the same as *... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking that **.md should actually produce a runtime error.

Yes, i think so, runtime error is better, in fact, i never use like Dir.glob("docs/**.md") before.

Copy link
Member

@straight-shoota straight-shoota Aug 9, 2022

Choose a reason for hiding this comment

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

No, we should not change established semantics. The glob patterns are not our own invention, they work the same way as in bash and multiple other shells and glob implementations.
ls docs/**.md in bash lists exactly the same paths as Dir.glob("docs/**.md").

This is completely valid though. ** only has a special meaning as a recursive match when it's followed by a path separator (/). But with out that, it's just consecutive wildcards, each matching a string of any length. That's completely valid syntax and has a clear meaning. It doesn't make a semantic difference to using a single wildcard. But that's not a reason to make it a syntax error. Especially when the same thing works in all other glob implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the first thought is, we don't like duplicate, if docs/*.md work same as docs/**.md, why we let user select?

talking about convention, i think the really big issue(for newcomer) is the " and ', when migrate one Ruby project to Crystal, probably the most annoy(boring) work is change ' to ". for least surprised, i still propose make double and single quotes function same in 2.0?, we can introduce ?A(ruby) or $'A'(bash) to present the Char type.

@zw963
Copy link
Contributor Author

zw963 commented Sep 18, 2022

This documentation improvement has not been merged yet ...

@straight-shoota
Copy link
Member

Yeah, it still needs a second approval from a Core Team member.

@zw963 zw963 mentioned this pull request Sep 18, 2022
@zw963
Copy link
Contributor Author

zw963 commented Sep 18, 2022

Yeah, it still needs a second approval from a Core Team member.

Hi, can i know for those delayed PR, because git HEAD is keeping moving, current old PR between newest master was forked, What is needs to be done before merging?

When this case happen, i usually do a rebase on my fork, and push -f instead, especially when there is a conflict with newest master, i guess this is not the expected way, what is the preferred way to do this for crystal-lang? e.g. for this PR.

@straight-shoota
Copy link
Member

If there are conflicts with master, you should merge master into the PR branch and resolve the conflicts in that merge (see https://github.com/crystal-lang/crystal/blob/master/CONTRIBUTING.md#making-good-pull-requests).

But you don't need to do anything here. Master has progressed, but there's no conflict.

@zw963
Copy link
Contributor Author

zw963 commented Sep 18, 2022

But you don't need to do anything here. Master has progressed, but there's no conflict.

@straight-shoota , one more question please, if the master is keep moving, which cause this old PR is not fast-forword, right?

if merge this branch into master after several months, which will cause a big merge graph, you know, like following screenshot.

image

This is acceptable, right?

@straight-shoota
Copy link
Member

We'll do a squash merge, so the entire branch will be reduced to a single commit. It's not fast-forward of course, but the history is linear.

@straight-shoota straight-shoota added this to the 1.6.0 milestone Sep 18, 2022
@straight-shoota straight-shoota added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Sep 20, 2022
@straight-shoota straight-shoota merged commit c7d90c5 into crystal-lang:master Sep 20, 2022
@zw963 zw963 deleted the docs/file branch September 20, 2022 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:docs topic:stdlib:files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants