Test multi-offenses for Layout/DotPosition cop

[tdeo]

Hi,

I'd like to show an issue I've been having with multi-offenses autocorrect for the Layout/DotPosition cop, I have a potential fix coming in a second commit.

In some cases, when having multiple offenses for this cop, it only autocorrects the first it finds and exits, which is very painful when relying on auto-correct to change from one style to another.

I'm not familiar with the ConfigurableEnforcedStyle, but my guess is that it is used for guessing the best style to use when auto-generating a config file.

I've found accross the code multiple use of both the pattern return unless opposite_style_detected and simple calls to opposite_style_detected:

$ git grep opposite_style_detected
lib/rubocop/cop/layout/access_modifier_indentation.rb:74:                opposite_style_detected
lib/rubocop/cop/layout/case_indentation.rb:125:            opposite_style_detected
lib/rubocop/cop/layout/dot_position.rb:34:          opposite_style_detected
lib/rubocop/cop/layout/space_around_equals_in_parameter_default.rb:65:            return unless opposite_style_detected
lib/rubocop/cop/layout/space_inside_block_braces.rb:168:                opposite_style_detected
lib/rubocop/cop/layout/space_inside_block_braces.rb:185:                opposite_style_detected
lib/rubocop/cop/layout/space_inside_block_braces.rb:209:            return unless opposite_style_detected
lib/rubocop/cop/layout/space_inside_block_braces.rb:219:            return unless opposite_style_detected
lib/rubocop/cop/mixin/configurable_enforced_style.rb:7:      def opposite_style_detected
lib/rubocop/cop/mixin/multiline_expression_indentation.rb:99:            opposite_style_detected
lib/rubocop/cop/mixin/string_help.rb:17:          add_offense(node) { opposite_style_detected }
lib/rubocop/cop/style/access_modifier_declarations.rb:90:            add_offense(node.loc.selector) if opposite_style_detected
lib/rubocop/cop/style/character_literal.rb:45:        def opposite_style_detected; end
lib/rubocop/cop/style/for.rb:52:            return unless opposite_style_detected
lib/rubocop/cop/style/for.rb:66:            return unless opposite_style_detected
lib/rubocop/cop/style/hash_syntax.rb:163:                opposite_style_detected
lib/rubocop/cop/style/ip_addresses.rb:42:        def opposite_style_detected; end
lib/rubocop/cop/style/lambda_call.rb:30:          if offense?(node) && opposite_style_detected
lib/rubocop/cop/style/raise_args.rb:87:            return unless opposite_style_detected
lib/rubocop/cop/style/raise_args.rb:106:          return unless opposite_style_detected
lib/rubocop/cop/style/special_global_vars.rb:123:            opposite_style_detected

Is there a practice that should be enforced for using opposite_style_detected ?

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[dvandersluis]

I've gotten confused from ConfigurableEnforcedStyle as well and I think given how widespread its use is, it would really benefit from documentation from someone who knows what's going on and how to properly use it.

[dvandersluis]

You can use expect_correction in your new test on the cop's spec rather than having it be tested through the CLI spec.

[tdeo]

Maybe @jonas054 could share some information about the expected use of the ConfigurableEnforcedStyle mixin ?

[tdeo]

Hi there, is there anything I can do to get this moving forward ? Auto-correction working only one line at a time Layout/DotPosition makes the process of standardizing the existing codebase quite painful

[bbatsov]

Sorry about the delay! I've been pretty busy with other things.

I'm not familiar with the ConfigurableEnforcedStyle, but my guess is that it is used for guessing the best style to use when auto-generating a config file.

Yeah, exactly.

Is there a practice that should be enforced for using opposite_style_detected ?

If people make mistakes I guess there should be.

[tdeo]

Sorry about the delay! I've been pretty busy with other things.

Thanks for merging and cutting the release

If people make mistakes I guess there should be.

Happy to improve this (quick) fix when there is some guidelines

[jonas054]

@tdeo @dvandersluis I understand that you're confused about the use of opposite_style_detected. It's meant to be called in cops that have exactly two supported styles in their configuration when an offense is detected but the inspected code snippet conforms to the opposite style, not the style not chosen. The purpose of this is to enable generation of EnforcedStyle parameters for .rubocop_todo.yml when there's a consistent use of a style, but it's not the style chosen in configuration.

There are two places in the code base where the method is used in conditions. This way of calling it is wrong, and it just happens to work in those two places because opposite_style_detected always returns a truthy value. Note that the method name doesn't end with a question mark. It's not a predicate.

Looking at ConfigurableEnforcedStyle again now, I remember that we wanted to make it smarter and better a long time ago, but it got stuck in this first, simple implementation. Ideas for improvement are:

• Producing EnforcedStyle parameters should work when there are more than two SupportedStyles.
• The logic for when to call methods in the ConfigurableEnforcedStyle mixin should be moved to a central location, and should not be the responsibility of individual cops.

[dvandersluis]

@jonas054 I think there were a bunch of PRs that removed it from some cops a month or two ago because there were problems where multiple offenses wouldn't be detected after opposite_style_detected was returned. I can't seem to find any in a quick search but maybe someone else remembers?

[jonas054]

I found #8759 #8799 #8801 in the logs (git log -S opposite_style_detected). The fixes for those issues all mention misuse of opposite_style_detected as the reason behind an observed problem.