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 Globber.constant_entry? matching patterns #13955

Conversation

GeopJr
Copy link
Contributor

@GeopJr GeopJr commented Nov 5, 2023

fix: #13954

This PR adds '{', '[', '\\' to Globber's constant_entry? so it matches patterns that include them without * or ?

@GeopJr
Copy link
Contributor Author

GeopJr commented Nov 5, 2023

Not sure if the spec files will cause issues on windows since they have backslashes and asterisks in their filenames

edit: they did

@GeopJr
Copy link
Contributor Author

GeopJr commented Nov 6, 2023

@HertzDevil
Copy link
Contributor

HertzDevil commented Nov 17, 2023

The Dir.glob specs use datapath from spec/std/spec_helper.cr, which always returns a native path. Previously things would continue to work as long as the constant directories are still separated from the rest of the segments using /:

Dir.glob(%q(spec/std/data/dir\*.txt)) # => []
Dir.glob(%q(spec\std\data\dir/*.txt)) # => ["spec\\std\\data\\dir\\f1.txt", "spec\\std\\data\\dir\\f2.txt", "spec\\std\\data\\dir\\g2.txt"]

But if Dir.glob uses the same patterns as File.match? and the docs already say that \ is not a directory separator, then those specs are indeed using datapath incorrectly. All those specs must be modified:

# okay
Dir["#{Path[datapath].to_posix}/dir/*.txt"].sort.should eq [
  datapath("dir", "f1.txt"),
  datapath("dir", "f2.txt"),
  datapath("dir", "g2.txt"),
].sort

Note that this pertains to String arguments only. Dir.glob converts Path arguments like this, something File.match? doesn't do:

crystal/src/dir/glob.cr

Lines 153 to 155 in 72e6b02

if pattern.is_a?(Path)
pattern = pattern.to_posix.to_s
end

src/dir/glob.cr Outdated Show resolved Hide resolved
@GeopJr
Copy link
Contributor Author

GeopJr commented Nov 18, 2023

Thanks for the info! I converted them to Path[datapath].to_posix

@GeopJr GeopJr marked this pull request as ready for review November 18, 2023 13:38
@GeopJr GeopJr marked this pull request as draft November 18, 2023 23:31
@GeopJr
Copy link
Contributor Author

GeopJr commented Nov 18, 2023

This seems to affect stuff outside the glob spec. I don't think I can do much without a Windows machine, feel free to take over this PR!

@HertzDevil HertzDevil self-assigned this Nov 19, 2023
@HertzDevil HertzDevil marked this pull request as ready for review November 20, 2023 10:39
src/compiler/crystal/command/format.cr Outdated Show resolved Hide resolved
src/compiler/crystal/command/spec.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.11.0 milestone Nov 26, 2023
@straight-shoota straight-shoota merged commit a59b5ab into crystal-lang:master Nov 27, 2023
54 of 55 checks passed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Co-authored-by: Quinton Miller <nicetas.c@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Globber incorrectly treats patterns without * or ? as constant entries
3 participants