-
-
Notifications
You must be signed in to change notification settings - Fork 278
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
Add new RSpec/FileFixture
cop
#1214
Conversation
`RSpec` provides the convenience method ```ruby file_fixture('path/some_file.csv') ``` which (by default) is equivalent to ```ruby Rails.root.join('spec/fixtures/files/path/some_file.csv') ``` We add a cop which detects and autocorrects verbose `Rails.root` constructions and works best in combination with [`Rails/RootPathnameMethods`](rubocop/rubocop-rails#587).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general.
Before looking more closely: do you think this cop will suit rubocop-rails
more?
Even though it's related to testing, it would also register offences in Minitest tests, wouldn't it?
And it's strictly for Rails.
@@ -0,0 +1 @@ | |||
* [#1214](https://github.com/rubocop/rubocop-rails/pull/1214): Add new `RSpec/FileFixture` cop. ([@leoarnold][]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still using the old approach with just one CHANGELOG.md in this repo - it's not a major pain to merge it.
# Rails.root.join('spec', 'fixtures', 'files', 'some_file.csv') | ||
# | ||
# # good | ||
# file_fixture('../path/some_file.csv').path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is path
a Pathname
method? I couldn't find it.
def_node_matcher :rails_root_join, <<~PATTERN | ||
(send | ||
(send | ||
(const {nil? cbase} :Rails) :root) :join (str $_)+) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if it's not a literal string passed to join
? E.g. a string variable or an interpolated string.
it 'registers an offense when using ' \ | ||
'`"\#{Rails.root}/spec/fixtures/files/some_file.csv"`' do | ||
expect_offense(<<~RUBY) | ||
"\#{Rails.root}/spec/fixtures/files/some_file.csv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible to write heredoc as <<~'RUBY'
to avoid escaping the hash.
|
||
RSpec.describe RuboCop::Cop::RSpec::FileFixture, :config do | ||
it 'registers an offense when using ' \ | ||
'`"\#{Rails.root}/spec/fixtures/files/some_file.csv"`' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docstring just duplicates what the example itself tells. Does it make sense to write a free-form text explaining this case?
@@ -353,6 +353,12 @@ RSpec/ExpectOutput: | |||
VersionAdded: '1.10' | |||
StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ExpectOutput | |||
|
|||
RSpec/FileFixture: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have Rails sub-department.
@pirj You're right. I thought RSpec had it's own I'll close here and reopen on |
RSpec
provides the convenience methodwhich (by default) is equivalent to
We add a cop which detects and autocorrects verbose
Rails.root
constructions and works best in combination with
Rails/RootPathnameMethods
.Before submitting the PR make sure the following are checked:
master
(if not - rebase it).CHANGELOG.md
if 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).If you have created a new cop:
config/default.yml
.Enabled: pending
inconfig/default.yml
.VersionAdded
indefault/config.yml
to the next minor version.If you have modified an existing cop's configuration options:
VersionChanged
inconfig/default.yml
to the next major version.