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 FilePath detection when absolute path includes test subject #869

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

eitoball
Copy link
Contributor

@eitoball eitoball commented Feb 6, 2020

This PR should fix FilePath cop when test subject is accidentally included in absolute path of file that being examined. For example, if file contains following spec,

RSpec.describe Foo do; end

and the absolute path name is like /home/foo/src/spec/models/bar_spec.rb.

I think this case should be warned, but currently it is passed because glob pattern foo*_spec.rb matches.

This PR changes to use relative path from project root so that it wouldn't match this case.


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Nice!
Will you please add a spec for this change?

@eitoball eitoball changed the title Fix FilePath cop when absolute path includes test subject Fix FilePath detection when absolute path includes test subject Feb 6, 2020
@eitoball eitoball force-pushed the use-relative-path-for-file_path-cop branch 6 times, most recently from e7ec9b3 to f91e85b Compare February 7, 2020 00:32
@eitoball
Copy link
Contributor Author

eitoball commented Feb 7, 2020

Will you please add a spec for this change?

Added. But, it's quite dirty...

@eitoball eitoball marked this pull request as ready for review February 7, 2020 00:35
spec/rubocop/cop/rspec/file_path_spec.rb Outdated Show resolved Hide resolved
@@ -103,7 +103,9 @@ def ignore_methods?
end

def filename_ends_with?(glob)
File.fnmatch?("*#{glob}", processed_source.buffer.name)
filename =
RuboCop::PathUtil.relative_path(processed_source.buffer.name)
Copy link
Member

Choose a reason for hiding this comment

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

It's possible to call rubocop from a subdirectory, e.g. /home/foo/spec/models/, in this case relative path will be ./bar_spec.rb, this shouldn't cause any thouble, should 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.

I think that it depends. If subject is like 'Foo::Bar', it wouldn't detect. I think that most people run from project root

Copy link
Member

Choose a reason for hiding this comment

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

Seems this way it doesn't work if I run rubocop and provide a path to check, e.g.
rubocop --require rubocop-rspec ~/my_project/specs

Copy link
Member

Choose a reason for hiding this comment

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

@eitoball Can you please check this edge case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't notice. Will do over this weekend. Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

A friendly reminder. This is ready to be merged apart from this edge case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi guys,
i think i had this edge case. I tried to create a pull request for it:

#917

@eitoball eitoball force-pushed the use-relative-path-for-file_path-cop branch 2 times, most recently from 03dddb8 to 9030ad4 Compare February 8, 2020 08:34
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@pirj
Copy link
Member

pirj commented Feb 12, 2020

@eitoball Can you please rebase?

@bquorning @Darhazer @lazycoder9 What are your thoughts on this?

@eitoball eitoball force-pushed the use-relative-path-for-file_path-cop branch from 9030ad4 to 2126827 Compare February 12, 2020 01:34
@eitoball
Copy link
Contributor Author

@pirj Rebased.

@eitoball eitoball force-pushed the use-relative-path-for-file_path-cop branch from 2126827 to 811ae08 Compare February 18, 2020 11:34
@pirj pirj requested review from Darhazer and bquorning February 24, 2020 11:19
@pirj pirj merged commit 84cca39 into rubocop:master Mar 25, 2020
@pirj
Copy link
Member

pirj commented Mar 25, 2020

Thanks a lot for your contribution!
If there's something left to be done in terms of running rubocop from a nested folder, please feel free to send a follow-up. No obligations, no rush.

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.

5 participants