cop_helpers should be deprecated

[marcandre]

As a followup to #7970, I feel that all of the methods in cop_helper should be deprecated. Here is why:

inspect_source and autocorrect_source seem too low level and don't lead to the right patterns. Among other things, they don't use lets for anything. So some things get duplicated (e.g. building the processed_source). No instance is shared by default, so many specs are calling inspect_source and then autocorrect_source, which is a really bad pattern: the same cop is called twice, with different source_buffer (although the same source code), with settings that has been hacked (via instance_variable_get 😱), something that won't even work currently if the cop is constructed with auto_correct set to true.

Example: this spec checks 78 cases (59 with offenses, 19 without). The inspect_source is called 118 times for nothing. Each case with offense also calls autocorrect_source twice.

I feel these (at least) should be officially deprecated (at least in the doc) in 1.0, in favor of expect_offense/expect_no_offense or better.

inspect_source_file is used in one spec, I don't know if there's a good reason for it at all.

Maybe parse_source could remain as is, although it is still very low level and I'd still favor processed_source (in shared context).

autocorrect_source_with_loop => I checked the one cop spec that uses it. Unless I'm mistaken, this isn't really about the cop but simply that some autocorrected cases may lead to another autocorrectable offense. Would be best handled with two expect_offense with correction or similar.

[bbatsov]

No objections from me. Those are all remnants from the early days of RuboCop and we certainly have better APIs at our disposal today.

[bquorning]

autocorrect_source_with_loop => I checked the one cop spec that uses it. Unless I'm mistaken, this isn't really about the cop but simply that some autocorrected cases may lead to another autocorrectable offense. Would be best handled with two expect_offense with correction or similar.

I am actually working on adding (optional) looping to the expect_correction method.

[marcandre]

I am actually working on adding (optional) looping to the expect_correction method.

Sounds great!

[rrosenblum]

I originally posted this info in #8127. I think it makes more sense here since it is a more technical discussion.

I haven't found a way to use expect_offense on any test that points to blank line. expect_offense doesn't have a way to process highlight that doesn't really point to anything https://github.com/rubocop-hq/rubocop/blob/3b0552b9989db97104cfe0fc30515e9b6ae549f2/spec/rubocop/cop/layout/empty_lines_around_method_body_spec.rb#L42-L51

  it 'registers an offense for class method body starting with a blank' do
    expect_offense(<<~RUBY)
      def Test.some_method

      ^ Extra empty line detected at method body beginning.
        do_something
      end
    RUBY
  end

Failures:

  1) RuboCop::Cop::Layout::EmptyLinesAroundMethodBody registers an offense for class method body starting with a blank
     Failure/Error: expect(actual_annotations.to_s).to eq(expected_annotations.to_s)

       expected: "def Test.some_method\n\n^ Extra empty line detected at method body beginning.\n  do_something\nend\n"
            got: "def Test.some_method\n\n Extra empty line detected at method body beginning.\n  do_something\nend\n"

       (compared using ==)

       Diff:
       @@ -1,6 +1,6 @@
        def Test.some_method

       -^ Extra empty line detected at method body beginning.
       + Extra empty line detected at method body beginning.
          do_something
        end

     # ./lib/rubocop/rspec/expect_offense.rb:125:in `expect_offense'
     # ./spec/rubocop/cop/layout/empty_lines_around_method_body_spec.rb:43:in `block (2 levels) in <top (required)>'

Finished in 0.04196 seconds (files took 1.03 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/rubocop/cop/layout/empty_lines_around_method_body_spec.rb:42 # RuboCop::Cop::Layout::EmptyLinesAroundMethodBody registers an offense for class method body starting with a blank

I think there might be a couple of other oddities to work through in order to convert everything. It's been a while since I worked on converting tests.

It might be worth while to document some of the tricks to handling white space and interpolation with different types of HEREDOCS. For instance,

    <<~RUBY
      puts 'foo' +
           "#{bar}"
      puts 'a' +
        'b'
      "#{c}"
    RUBY

# vs

    # The markdown syntax highlighting is incorrect here. 
    # This does not actually perform string interpolation
    <<~'RUBY'
      puts 'foo' +
           "#{bar}"
      puts 'a' +
        'b'
      "#{c}"
    RUBY

[marcandre]

A bit cryptic, but we could use ^{} to signify en empty marker here, or another special character.

[biinari]

I like ^{} as it reflects the existing ^ and ^{name} format. I've submitted a PR with this but happy to change it up if there are any preferred suggestions.