[Fix #3779] Add Rails/FilePath cop #3822
Conversation
| subject(:cop) { described_class.new } | ||
|
|
||
| context 'when Rails.root.join with some path strings' do | ||
| let(:source) { 'Rails.root.join(\'app\', \'models\', \'user.rb\')' } |
There was a problem hiding this comment.
I had this exact thing in a code review last week, so I guess I agree that Style/StringLiterals should account for this. 😅
There was a problem hiding this comment.
I'll create this issue!
|
|
||
| it 'registers an offense' do | ||
| inspect_source(cop, source) | ||
| expect(cop.offenses.size).to eq(0) |
There was a problem hiding this comment.
Prefer the usage of be_empty.
| module RuboCop | ||
| module Cop | ||
| module Rails | ||
| # @example |
There was a problem hiding this comment.
I'd also add some general explanations here.
| context 'when Rails.root.join with some path strings' do | ||
| let(:source) { 'Rails.root.join(\'app\', \'models\', \'user.rb\')' } | ||
|
|
||
| it 'registers an offense' do |
| describe RuboCop::Cop::Rails::FilePath do | ||
| subject(:cop) { described_class.new } | ||
|
|
||
| context 'when Rails.root.join with some path strings' do |
| end | ||
| end | ||
|
|
||
| context 'when File.join with Rails.root' do |
| offense(node) | ||
| end | ||
|
|
||
| def offense(node) |
There was a problem hiding this comment.
I'd name this register_offense or something like this.
|
|
||
| private | ||
|
|
||
| def file_join_with_rails_root(node) |
There was a problem hiding this comment.
I'd name this check_for_file_join_with_rails_root or something else. Or maybe you could extract a separate predicate method from this one to simplify the code?
| offense(node) | ||
| end | ||
|
|
||
| def rails_root_join_with_slash_separated_path(node) |
|
You should also write a matching rule in the Rails style guide. |
OK, I will. https://github.com/bbatsov/rails-style-guide#table-of-contents |
|
Thank you for great reviews and i fixed all of review comment point. |
Guess such code most often comes up in configuration, so I guess it can be in this section... |
|
Can someone please explain to me why this:
Is better than this:
Surely the latter is shorter, easier to read, and better on memory (allocates fewer strings). Thanks |
|
@Bodacious You're hard coding the path separator. It's reasonable assumption that it will be a "/" but not 100% universal. One somebody might want to run your code on Windows or a future OS that doesn't use "/". Regarding the ugliness of the first format, I sometimes use which, while still a bit ugly, is at least shorter. (i.e. it will handle an array while |
|
@dominicsayers: Care to open a new request to add a configuration option for the path separator? 🙂 |
|
You mean remove the hard-coded forward slash from the cop? That wouldn't need a configuration option, just replace the slash with EDITED TO ADD: ...although making a regex with |
|
@dominicsayers thanks for taking the time to explain it. I never considered OS that don't use "/" 👍 |
Ah. Yes. Of course. I think we have a Windows CI now, so I might give it a shot. |
Add Rails/FilePath cop
Related issue: #3779
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).and description in grammatically correct, complete sentences.
rake generate_cops_documentation(required only when you've added a new cop or changed the configuration/documentation of an existing cop).