Skip to content
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

RuboCop exception handling of autocorrecting cop has multiple issues #8016

Closed
marcandre opened this issue May 23, 2020 · 1 comment
Closed
Assignees

Comments

@marcandre
Copy link
Contributor

marcandre commented May 23, 2020

While working on #7868, I realized that RuboCop's exception handling of autocorrecting cop has issues:

  1. some exceptions may be completely ignored:
# my_cop.rb:
module Custom
  class MyCop < RuboCop::Cop::Cop
    MSG = "let's be positive"

    def on_send(node)
      if node.method_name == :-
        add_offense(node)
        raise 'oh!'  # <<< Completely ignored!
      end
    end

    def autocorrect(node)
      -> (corrector) do
        corrector.replace(node.loc.selector, '+')
      end
    end
  end
end
> echo "2-2" | rubocop -r ./my_cop.rb --only Custom -a -s "test"

Inspecting 1 file
C

Offenses:

test:1:1: C: [Corrected] Custom/MyCop: let's be positive
2-2
^^^

1 file inspected, 1 offense detected, 1 offense corrected
====================
2+2
  1. as noted in Improving auto correction's API #7868, some exceptions will crash rubocop (move the raise inside the -> in the example above)

There are 3 specs for this handling in team_spec, but 1 is redundant with the previous spec, and there are quite a few cases missing and don't test the interaction with Runner. For example this spec fails if we add let(:options) { { auto_correct: true } }.

I don't intend on cleaning this up before #7868 is completed. It should be easier to deal with too as the current autocorrection timing is quite intricate.

@marcandre marcandre self-assigned this May 23, 2020
marcandre added a commit to marcandre/rubocop that referenced this issue May 23, 2020
It tests the same thing as the previous spec; their success is not independent.
See [rubocop#8016]
marcandre added a commit to marcandre/rubocop that referenced this issue May 23, 2020
marcandre added a commit to marcandre/rubocop that referenced this issue May 30, 2020
It tests the same thing as the previous spec; their success is not independent.
See [rubocop#8016]
marcandre added a commit to marcandre/rubocop that referenced this issue May 30, 2020
marcandre added a commit to marcandre/rubocop that referenced this issue May 31, 2020
It tests the same thing as the previous spec; their success is not independent.
See [rubocop#8016]
marcandre added a commit that referenced this issue May 31, 2020
It tests the same thing as the previous spec; their success is not independent.
See [#8016]
@marcandre
Copy link
Contributor Author

I believe this to be fixed in #7868

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant