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

[Fix #8663] Fix issues with Style/ClassMethodsDefinitions #8687

Merged
merged 2 commits into from Sep 15, 2020

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Sep 10, 2020

I found quite a few issues with autocorrection in Style/ClassMethodsDefinitions, that are now handled, with fuller test coverage.

  • More than one method within a class << self block is now handled correctly (previously it would duplicate methods)
  • Empty self << class blocks after autocorrection are now removed.
  • Extra whitespace around newly written methods is removed.
  • New methods are indented at the same level class << self was.

Fixes #8663.


Previously each def within class << self was assigned a separate offense, but this was causing a number of issues that when I tried to fix I was getting a clobbering error (for instance, source_range_with_comment in CommentsHelp was returning the entire body of the sclass, rather than just a single def node + comments, which was leading to duplicated methods).

I could not find a way to add offenses on each def but only autocorrect once, so I decided to move the offense to the sclass itself. I hope this is an okay tradeoff; please let me know if there's a better way.

I am not 100% satisfied with the amount of steps necessary in the corrector but I could not find a simpler solution that satisfied all the things I was trying to do (correct indentation, remove extra lines, and remove the empty class << self). Again if there is a suggestion for a better corrector, please let me know!


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.

Comment on lines -10 to +12
begin_pos, end_pos =
if node.def_type?
start_node = find_visibility_start(node) || node
end_node = find_visibility_end(node) || node
[begin_pos_with_comment(start_node),
end_position_for(end_node) + 1]
else
[begin_pos_with_comment(node), end_position_for(node)]
end
begin_pos = begin_pos_with_comment(node)
end_pos = end_position_for(node)
end_pos += 1 if node.def_type?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, this method was incorrectly getting a range for not just the def, but all defs within the same visibility group. I've fixed it to just return the def and its preceding comments (this mixin is only in use for this cop, so this should not affect anything else).

@dvandersluis dvandersluis force-pushed the fix-class-methods-definitions branch 2 times, most recently from 0f62676 to 3696bcf Compare September 10, 2020 20:14
Autocorrection was rewritten, which notably moves the offense for def_self style to the sclass itself, in order to properly handle each method within it without getting clobbering errors.

Autocorrection fixes:
* Handle multiple methods within class << self
* Remove extra whitespace
* Fix indentation
* Remove self << class if empty
@dvandersluis dvandersluis force-pushed the fix-class-methods-definitions branch 2 times, most recently from e2c15bf to 122e04c Compare September 11, 2020 20:07
@bbatsov
Copy link
Collaborator

bbatsov commented Sep 14, 2020

I could not find a way to add offenses on each def but only autocorrect once, so I decided to move the offense to the sclass itself. I hope this is an okay tradeoff; please let me know if there's a better way.

That's my only concern about the PR, but I feel that the tradeoff makes sense. @koic @marcandre Any thoughts from you?

@marcandre
Copy link
Contributor

I could not find a way to add offenses on each def but only autocorrect once, so I decided to move the offense to the sclass itself. I hope this is an okay tradeoff; please let me know if there's a better way.

That's my only concern about the PR, but I feel that the tradeoff makes sense. @koic @marcandre Any thoughts from you?

That's a reasonable way to do it. OTOH, it should be reasonably easy to add offenses to the def nodes:

def on_new_investigation
  @already_corrected_classes = Set[]
end

# ...
  add_offense(def_node) {
    class_node = class_for_def(def_node)
    next unless @already_corrected_classes.add?(class_node)

    # actual auto-correction
  end

@dvandersluis
Copy link
Member Author

@marcandre I wanted to collect the defs so I could move them out of the class << self and spread them out properly which I couldn’t figure out when moving them one at a time (and I was getting clobbering errors too).

If you want me to try again I will but if it is a reasonable solution as-is, that’s ok by me 😅

@marcandre
Copy link
Contributor

Oh, sorry, I didn't understand completely.

I'm surprised you got clobbering errors, as I imagine your corrections were a bunch of remove of the original defs and a bunch of insert of the modified def after the sclass' end. The corrector should have no issue inserting texts at the same location a bunch of times.

The main difference for the user is where to put rubocop:disable. It doesn't sound too bad that the decision has to be per sclass and not per def.

In short: I'm 👍 either way. If you prefer the multiple offense route and run into difficulties with clobbering, I can have a look.

@bbatsov bbatsov merged commit 295b9a6 into rubocop:master Sep 15, 2020
@dvandersluis
Copy link
Member Author

@marcandre I think it was because I was trying to keep the defs in the same order as they originally were in and also retain the new lines between them. Could be I was doing something wrong though.

Looks like @bbatsov merged it — thanks! — but if you want me to take another stab at it let me know.

@dvandersluis
Copy link
Member Author

Oh and the other problem was removing the empty sclass when the offenses/autocorrecting was per def.

@marcandre
Copy link
Contributor

@marcandre I think it was because I was trying to keep the defs in the same order as they originally were in and also retain the new lines between them. Could be I was doing something wrong though.

Right, I imagine it can be a bit tricky.

Oh and the other problem was removing the empty sclass when the offenses/autocorrecting was per def.

"swallowed deletions" are ignored so should not have been an issue to delete it on the last offense.

Looks like @bbatsov merged it — thanks! — but if you want me to take another stab at it let me know.

We're all glad it works, that's the most important! Thanks for this 👍

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 15, 2020

Looks like @bbatsov merged it — thanks! — but if you want me to take another stab at it let me know.

Up to you. I merged it mostly because I plan a new release later today and even with some quirks that PR improves what we had before. It's never late for more improvements. :-) Thanks for tackling this!

@dvandersluis dvandersluis deleted the fix-class-methods-definitions branch January 18, 2021 20:36
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

Successfully merging this pull request may close these issues.

Style/ClassMethodsDefinitions is wrongly creating an instance method
3 participants