-
-
Notifications
You must be signed in to change notification settings - Fork 263
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 Rails/RootPathnameMethods
cop
#587
Conversation
88fffc7
to
742cfb5
Compare
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.
Nice, thank you!
A few cosmetic notes. Wondering if those different node patterns can be collapsed into one.
`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).
…tion-on-belongs_to [Fix #587] Add Rails/RedundantPresenceValidationOnBelongsTo cop
@leoarnold ping :-) |
c9e45ad
to
ef3f050
Compare
@koic Thanks for the ping. I resolved all comments. Yet there is one very peculiar problem:
|
b54990e
to
5e3303d
Compare
5e3303d
to
d3ef7b3
Compare
unlink | ||
].to_set.freeze | ||
|
||
FILE_METHODS = %i[ |
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.
File.methods - File.superclass.methods
also returns e.g. :absolute_path
which is not on those lists.
Which makes me think does it make sense to fill in those constants with those calls?
On the other hand, if the Ruby version with which rubocop
is run is different from the target Ruby version, there might be differences.
However, does it matter much?
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.
... but Pathname
does not have a #absolute_path
method. Not sure I can follow here.
I generated this list using
File.methods.sort.select do |m|
Pathname.instance_methods.include?(m) && !Object.instance_methods.include?(m)
end
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.
Understood.
So basically (File.public_methods(false) & Pathname.instance_methods(false)).sort
.
Does it make sense to set the constant to this statement?
What are the consequences? New methods being added on File
and to Pathname
in future Ruby versions change the behaviour of the cop?
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.
Well, just because there is File.foo
and Pathname#foo
, it is not guaranteed that File.foo(p, ...)
and p.foo(...)
is the same, so I'd rather maintain a list of safe methods.
Also, there may be cases where the Ruby that runs Rubocop is not the Ruby which runs production code (e.g. you develop a gem on Ruby 2, but someone uses it on JRuby 9). Are those standard libraries necessarily equivalent?
|
||
RSpec.describe RuboCop::Cop::Rails::RootPathnameMethods, :config do | ||
{ | ||
Dir: described_class::DIR_METHODS, |
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.
It's my personal preference, but I don't feel too confident when some collections from the code are used in tests directly. It may sound far-fetched, but what if someone accidentally removes a line from that array? Specs won't fail.
I'll leave this to @koic to decide on.
d3ef7b3
to
b10c351
Compare
`Rails.root` is an instance of `Pathname`. So instead of ```ruby File.open(Rails.root.join('db', 'schema.rb')) File.open(Rails.root.join('db', 'schema.rb'), 'w') File.read(Rails.root.join('db', 'schema.rb')) File.binread(Rails.root.join('db', 'schema.rb')) File.write(Rails.root.join('db', 'schema.rb'), content) File.binwrite(Rails.root.join('db', 'schema.rb'), content) ``` we can simply write ```ruby Rails.root.join('db', 'schema.rb').open Rails.root.join('db', 'schema.rb').open('w') Rails.root.join('db', 'schema.rb').read Rails.root.join('db', 'schema.rb').binread Rails.root.join('db', 'schema.rb').write(content) Rails.root.join('db', 'schema.rb').binwrite(content) ``` This cop works best when used together with [`Style/FileRead`](rubocop/rubocop#10261), [`Style/FileWrite`](rubocop/rubocop#10260), and [`Rails/RootJoinChain`](rubocop#586).
b10c351
to
467da4e
Compare
It covers possible use cases and is maintainable. It will take a while to ship, so we can adjust it later if necessary. Thank you. |
Rails.root
is an instance ofPathname
. So instead ofwe can simply write
This cop works best when used together with
Style/FileRead
,Style/FileWrite
, andRails/RootJoinChain
.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.and description in grammatically correct, complete sentences.
bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.