ConditionalAssignment with assign_inside_condition

[ptarjan]

There are a few changes to assign_inside_condition that would be better for us:

1. We are ok with ternary assignment. This should be allowed

  @chromeless = params[:chromeless] ? :present : :missing

2. It crashes on a source file with this backtrace:

undefined method `loc' for nil:NilClass
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/util.rb:50:in `ternary_op?'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/style/conditional_assignment.rb:291:in `move_assignment_inside_condition'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/style/conditional_assignment.rb:269:in `autocorrect'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/cop.rb:179:in `correct'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/cop.rb:169:in `add_offense'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/style/conditional_assignment.rb:240:in `check_assignment_to_condition'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/style/conditional_assignment.rb:216:in `block (2 levels) in <class:ConditionalAssignment>'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:42:in `block (2 levels) in on_lvasgn'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:97:in `with_cop_error_handling'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:41:in `block in on_lvasgn'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:40:in `each'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:40:in `on_lvasgn'
(eval):2:in `block in on_begin'
(eval):2:in `each'
(eval):2:in `on_begin'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:46:in `on_begin'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/ast_node/traversal.rb:155:in `on_block'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:46:in `on_block'
(eval):2:in `block in on_begin'
(eval):2:in `each'
(eval):2:in `on_begin'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:46:in `on_begin'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/ast_node/traversal.rb:89:in `on_class'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:46:in `on_class'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/ast_node/traversal.rb:142:in `on_while'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:46:in `on_module'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/ast_node/traversal.rb:142:in `on_while'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:46:in `on_module'
(eval):2:in `block in on_begin'
(eval):2:in `each'
(eval):2:in `on_begin'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:46:in `on_begin'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/ast_node/traversal.rb:13:in `walk'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/commissioner.rb:59:in `investigate'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cop/team.rb:59:in `inspect_file'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/runner.rb:203:in `inspect_file'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/runner.rb:173:in `block in do_inspection_loop'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/runner.rb:163:in `loop'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/runner.rb:163:in `do_inspection_loop'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/runner.rb:87:in `process_file'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/runner.rb:59:in `block in inspect_files'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/runner.rb:57:in `each'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/runner.rb:57:in `inspect_files'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/runner.rb:35:in `run'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/lib/rubocop/cli.rb:30:in `run'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/bin/rubocop:14:in `block in <top (required)>'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/2.1.0/benchmark.rb:294:in `realtime'
/Users/pt/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rubocop-0.39.0/bin/rubocop:13:in `<top (required)>'
/Users/pt/.rbenv/versions/2.1/bin/rubocop:23:in `load'
/Users/pt/.rbenv/versions/2.1/bin/rubocop:23:in `<main>'

[ptarjan]

/cc @rrosenblum

[rrosenblum]

1. We are ok with ternary assignment. This should be allowed

@chromeless = params[:chromeless] ? true : false

assign_to_condition is designed to change params[:chromeless] ? @chromeless = true : @chromeless = false to @chromeless = params[:chromeless] ? true : false. When coding assign_inside_condition, I designed it to do the opposite of assign_to_condition. We could make the cop configurable to flag or not flag types of conditions (if, ternary, unless, and case).

2. It crashes on a source file with this backtrace

Can you please provide an example of some code that causes this error?

[defrex]

I would also love an option to enforce assign_inside_condition with the exception of ternary conditions. Indenting an entire if-else block is ugly IMO, but short ternary assignments are totally fine.

[rrosenblum]

@defrex, that sounds doable to me. I think the easiest way to implement this will be to add a config option outside of the SupportedStyles to include or disclude ternary conditions. That way it can work with assign_inside_condition or assign_to_condition.

This might take a little while to implement. That cop is very complex and probably the worst offender to any of the Metrics cops. I will try to get started on the configuration in the next couple of days.