Comments (7)
Yes, I understand that. What I'm saying is that we have two major choices for how to fix what's missing in RuboCop. Either we add expressions with &.
to the duties of Lint/AssignmentInCondition
, or we implement them in a new Lint/PossibleAssignmentToNil
cop.
Fixing Lint/AssignmentInCondition
has two drawbacks IMO.
- We're still not reporting an offense for
Topic.first.category&.name = 'Ruby 1'
when it's a statement of its own, even though the ampersand makes no sense. - We can't in good conscience autocorrect the
if
statement toif (Topic.first.category&.name = 'Ruby 1')
. That's a bit too unsafe, even for an autocorrection marked as unsafe. So we'd need to make it a special case, with extra logic in the cop.
from rubocop.
I take back most of my comments above. I was thinking of these expressions as assignments rather than setter calls. They are setter calls, so the fix becomes pretty simple. We just need to treat &.
the same as .
in the context of Lint/AssignmentInCondition
. A new PR is coming up.
from rubocop.
I find this example to be too contrived. Running the code produces this output:
$ ruby test.rb
test.rb:3:in `<main>': undefined method `to_i=' for 1:Integer (NoMethodError)
Did you mean? to_i
to_int
to_c
to_r
to_f
to_s
Can you give a better example where the inspected code is more realistic?
from rubocop.
Sorry for bad example, I will give a more realistic example.
test.rb
require 'bundler/inline'
gemfile(true) do
source 'https://rubygems.org'
gem 'activerecord', '~> 7.0', '>= 7.0.6', require: "active_record"
gem 'sqlite3', '~> 1.6', '>= 1.6.3'
gem 'minitest', '~> 5.19', require: "minitest/autorun"
end
ActiveRecord::Base.establish_connection(
adapter: "sqlite3",
database: "./test.db"
)
ActiveRecord::Schema.define do
drop_table(:categories, if_exists: true)
drop_table(:topics, if_exists: true)
create_table :categories do |t|
t.string :name
end
create_table :topics do |t|
t.belongs_to :category
t.string :title
t.text :content
end
end
class ApplicationRecord < ActiveRecord::Base
primary_abstract_class
end
class Category < ApplicationRecord
has_many :topics
end
class Topic < ApplicationRecord
belongs_to :category, optional: true
end
class MyTest < Minitest::Test
Category.create(name: 'Ruby')
Topic.create(title: 'Rubocop tutorial', category: Category.first)
def test_1
# rubocop will NOT fild this problem
if Topic.first.category&.name = 'Ruby 1'
puts 'test_1'
end
end
def test_2
# rubocop will find this problem
if Topic.first.category.name = 'Ruby 2'
puts 'test_2'
end
end
end
from rubocop.
root@dev-test:~$ ruby test.rb
Fetching gem metadata from https://gems.ruby-china.com/........
Resolving dependencies...
Using base64 0.2.0
Using bigdecimal 3.1.6
Using concurrent-ruby 1.2.3
Using drb 2.2.1
Using i18n 1.14.4
Using sqlite3 1.7.2 (x86_64-linux)
Using tzinfo 2.0.6
Using minitest 5.22.2
Using mutex_m 0.2.0
Using timeout 0.4.1
Using bundler 2.4.16
Using connection_pool 2.4.1
Using activesupport 7.1.3.2
Using activemodel 7.1.3.2
Using activerecord 7.1.3.2
-- drop_table(:categories, {:if_exists=>true})
-> 0.0139s
-- drop_table(:topics, {:if_exists=>true})
-> 0.0003s
-- create_table(:categories)
-> 0.0006s
-- create_table(:topics)
-> 0.0008s
Run options: --seed 48835
# Running:
test_2
.test_1
.
Finished in 0.006965s, 287.1380 runs/s, 0.0000 assertions/s.
2 runs, 0 assertions, 0 failures, 0 errors, 0 skips
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$ rubocop --only Lint/AssignmentInCondition -a test.rb
Inspecting 1 file
W
Offenses:
test.rb:56:34: W: [Correctable] Lint/AssignmentInCondition: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
if Topic.first.category.name = 'Ruby 2'
^
1 file inspected, 1 offense detected, 1 more offense can be corrected with `rubocop -A`
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$
root@dev-test:~$ ruby -v
ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-linux]
root@dev-test:~$ rubocop -v
1.61.0
root@dev-test:~$
from rubocop.
Thanks, @itsalongstory, much better!
An idea I'm getting when looking at the example is that it would be better to add a new cop that reports offenses for code like Topic.first.category&.name = 'Ruby 1'
because that code is very suspicious, even outside of the if
statement. The cop could be called Lint/PossibleAssignmentToNil
and report any use of &.
on the left hand side of an assignment, because I can't see why you would ever need it.
from rubocop.
I can't see why you would ever need it.
@jonas054 , the category
could be nil
class Topic < ApplicationRecord
belongs_to :category, optional: true
end
And my original intent was to determine equality, not to assign values.
from rubocop.
Related Issues (20)
- Style/InPatternThen crashes HOT 1
- Style/IdenticalConditionalBranches crashes HOT 1
- Style/RedundantRegexpArgument uses double quotes HOT 1
- Do not do unsafe autocorrection for `Lint/UselessAssignment` HOT 1
- False negative for `Lint/Void` with ternary expression HOT 2
- Style/ArgumentsForwarding drops arguments
- Autocorrect errors with Lint/ImplicitStringConcatenation and Style/StringConcatenation HOT 1
- Inconsistent `Style/ArgumentsForwarding` `...` forwarding behavior
- Enhancement: detect safe navigation in `Style/ReturnNilInPredicateMethodDefinition`
- "Style/MapIntoArray" false positive with push(*ary) HOT 2
- RuboCop spaces -> tab indentation generates overly indented output. HOT 6
- Cop idea: detect redundant setting to empty string inside string interpolation
- Sponsorship logo missing from README HOT 1
- Cops restricted by `Include` are not run when linting a single file HOT 1
- Style/Alias generates invalid code
- issue occur again and again HOT 1
- [Documentation] FAQ, in particular a "usage FAQ" and rubocop-quickref HOT 1
- Style/SymbolProc auto fix brakes logic HOT 3
- Style/RedundantCondition breaks code HOT 1
- Cop idea: lint for inconsistent safe navigation
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rubocop.