Fix false positive for relative file path runs with long namespaces (fixes #1091) #1099
Conversation
lib/rubocop/cop/rspec/file_path.rb
Outdated
| offense_suffix = pattern.gsub('.*', '*').gsub('[^/]*', '*') | ||
| .sub('\.rb', '.rb') |
There was a problem hiding this comment.
How about if we substitute as little as possible, so very narrowly matching the regex-specific parts of the string? E.g.
| offense_suffix = pattern.gsub('.*', '*').gsub('[^/]*', '*') | |
| .sub('\.rb', '.rb') | |
| offense_suffix = pattern.gsub('.*', '*').sub('[^/]', '') | |
| .sub('\.', '.') |
There was a problem hiding this comment.
Sure, it's changed now.
lib/rubocop/cop/rspec/file_path.rb
Outdated
| File.fnmatch?("*#{glob}", filename) | ||
| def filename_ends_with?(pattern) | ||
| filename = File.expand_path(processed_source.buffer.name) | ||
| filename.match?(".*#{pattern}$") |
There was a problem hiding this comment.
.* is unnecessary at the beginning (or end) of a regex.
There was a problem hiding this comment.
Could we move the $ (which should probably be \z?) into the pattern itself? Then this line would become simply filename.match?(pattern)…
There was a problem hiding this comment.
- You are right about unnecessary
.*in the beginning. It is removed. - We cannot move the
$or\zto thepattern_formethod because it would then appear in the offense messages unless it is also substituted. I think that it makes sense to keep the ending part of the pattern in the filename_ends_with? method. - Regarding
$or\zI guess either one is fine. Is there any case when a single filename can be multiline?
We don’t use GitHub’s “Squash and merge” option, since it fails to create a merge commit. |
There was a problem hiding this comment.
It's strange that the cop fails to detect an offence in:
RSpec.describe Object, "should" do
or
RSpec.describe Object, "#should" do
when inspecting spec/rspec/expectations/extensions/kernel_spec.rb.
But it was like that before this change as well.
Yet another case that this cop is missing (both before and after the change) is a namespace:
# spec/rspec/expectations/configuration_spec.rb
module RSpec
module Mocks # Should be Expectations
RSpec.describe Configuration do
Those two are not blockers, but more like ideas if you'd like to follow-up on this.
There's a new legitimate offence detected:
/Users/pirj/source/rspec-dev/repos/rspec-expectations/spec/rspec/expectations/syntax_spec.rb:88:3: C: RSpec/FilePath: Spec path should end with expectations*_spec.rb.
RSpec.describe Expectations do
^^^^^^^^^^^^^^^^^^^^^^^^^^^
👍
I couldn't break the cop by testing locally.
Looks good, the change is quite elegant, and besides fixing all the cases is now easier to read. Thanks a lot!
Do we have any Windows users around? |
|
Please add a Changelog entry, squash commits and I believe we're ready to merge. |
When rubocop is run in a relative path to a spec with long namespace, e.g. in the same directory, the FilePath spec will return incorrect matches. This fixes them by switching to a regular expression based pattern matching which allows more specific matchign than the previously used globs. Adds new specs to ensure the file extension is matched against the ruby file extension (.rb). Fixes #1091
|
@pirj Changelog added and commits are squashed. |
@bquorning I tested this under Windows and it seems to work fine there as is. This is because |
This is a PR that fixes the issue that is further explained at #1091.
This was broken by this commit included in the PR #869:
811ae08#diff-5b709885b35566a0c699ad4a3200b9b7acbc28ef640fa56a8e66f6c732420a34
It turned out to be extremely difficult (at least I don't know how after multiple tries and experiments) to fix the issue described at #1091 using the file name globs. Therefore I switched to regular expression based matching which allows a more specific targeting for the case in question.
The removed specs originate from this commit in the PR #917:
23ad402#diff-5bc1941f9b92914876efec6f7fcc70b153e82194d960213a280078c6cb4b0c3a
The removed specs are no longer needed because it now uses absolute paths as it did prior to merging #869. I believe these were also issues introduced by #869.
Fixes #1091
Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif the new code introduces user-observable changes.bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).