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

ConfigurationFileFinder: test magic paths #455

Merged
merged 1 commit into from
Apr 20, 2015

Conversation

chastell
Copy link
Collaborator

The fact that we couldn’t match the .reek config file via *.reek globbing was growing on me, so I dove into docs and found a way. It turns out the globbing doesn’t work 100% of the time in Rubinius, so this PR only adds the below:

I also fondly remember the wise words of Michael Grünewald:

Hobby: creating bug tickets, waiting for people to find out about my home directory – which happens to contain spaces, stars and newlines.

– so I added a relevant spec.

@chastell
Copy link
Collaborator Author

Well did I just find a Rubinius bug incompatibility improvement over MRI…

@chastell chastell force-pushed the simplify_ConfigurationFileFinder branch 2 times, most recently from dcc0155 to c28cca5 Compare April 18, 2015 19:47
@chastell
Copy link
Collaborator Author

Ok, so Rubinius 2.5.2 does pass the magic paths spec when we do the old way around, with

          files = dir.children.select(&:file?).sort
          found = files.find { |file| file.to_s.end_with?('.reek') }

rather than

          found = Pathname.glob("#{dir}/{.reek,*.reek}").sort.find(&:file?)

– so for now I reverted my globbing simplification and will revisit this if/when Rubinius acts the same as MRI and JRuby.

@chastell chastell force-pushed the simplify_ConfigurationFileFinder branch from c28cca5 to 50a5d85 Compare April 18, 2015 19:51
@chastell chastell changed the title Simplify ConfigurationFileFinder and test magic paths ConfigurationFileFinder: test magic paths Apr 18, 2015
@mvz
Copy link
Collaborator

mvz commented Apr 19, 2015

Interesting. Globbing in general seems to be a problematic thing. See for example rubinius/rubinius#3149 and https://bugs.ruby-lang.org/issues/10700.

Also, interpolating "ma\ngic-d*r" into a globbing pattern yields a pattern that will match `"ma\ngic-d-how-did-we-get-here-r". I don't think there's any ready made escape function we can apply to fix this.

My conclusion is that as soon as we make dir part of the globbing pattern, we're not guaranteed to get only files in dir, and we're better off with the current implementation.

@chastell chastell force-pushed the simplify_ConfigurationFileFinder branch from 50a5d85 to fae836f Compare April 19, 2015 07:52
@chastell
Copy link
Collaborator Author

Also, interpolating "ma\ngic-d*r" into a globbing pattern yields a pattern that will match "ma\ngic-d-how-did-we-get-here-r"

Oh, good point. I removed the FIXME with the globbing approach – I still think it’s worth to have this test, though.

@mvz
Copy link
Collaborator

mvz commented Apr 19, 2015

I still think it’s worth to have this test, though.

Definitely.

troessner pushed a commit that referenced this pull request Apr 20, 2015
ConfigurationFileFinder: test magic paths
@troessner troessner merged commit eebe157 into develop Apr 20, 2015
@troessner troessner deleted the simplify_ConfigurationFileFinder branch April 20, 2015 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants