Assignment of if result incorrectly autocorrected

[amw]

I got this bad autocorrect. The original code:

  def expression= value
    self[:expression_id] =
      if value.kind_of? Expression
        value.id
      else
        nil
      end
  end

Was changed to:

  def expression= value
    self[:expression_id] =
      value.id if value.is_a? Expression
end

This of course is incompatible. It makes the whole assignment conditional.

If you think this has to be corrected at all then you should use ternary operator:

  def expression= value
    self[:expression_id] = value.is_a?(Expression) ? value.id : nil
  end

[alexdowad]

This is caused by the Style/IfUnlessModifier cop. Style/EmptyElse is also involved, but the transform it does is valid.

[swatosh]

Unless self[:expression_id] has a value that should be overwritten with nil
On Oct 26, 2015 1:56 PM, "Alex Dowad" [email protected] wrote:

This is caused by the Style/IfUnlessModifier cop. Style/EmptyElse is also
involved, but the transform it does is valid.

—
Reply to this email directly or view it on GitHub
#2323 (comment).

[alexdowad]

Sorry, @swatosh, what do you mean?

[swatosh]

If self[:expression_id == 1234 before the method call, there is a code path
in the original that would change it to nil.

Sorry, @swatosh https://github.com/swatosh, what do you mean?

—
Reply to this email directly or view it on GitHub
#2323 (comment).

[alexdowad]

If self[:expression_id == 1234 before the method call, there is a code path
in the original that would change it to nil.

Yes. That is the whole point of the linked PR.

Currently, RuboCop removes the code path which sets self[:expression_id] to nil. With the linked PR merged, it will not do so.

[swatosh]

Sorry, I missed the PR. I was responding to "...but the transform it does
is valid." Again, sorry for misunderstanding.
On Oct 27, 2015 12:45 PM, "Alex Dowad" [email protected] wrote:

If self[:expression_id == 1234 before the method call, there is a code path
in the original that would change it to nil.

Yes. That is the whole point of the linked PR.

Currently, RuboCop removes the code path which sets self[:expression_id]
to nil. With the linked PR merged, it will not do so.

—
Reply to this email directly or view it on GitHub
#2323 (comment).

[amw]

@swatosh I think it meant that Style/EmptyElse transform is valid and only Style/IfUnlessModifier needs fixing.