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
[Fix #8663] Fix issues with Style/ClassMethodsDefinitions #8687
Conversation
3f878af
to
f2c3ff6
Compare
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? |
There was a problem hiding this comment.
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 def
s 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).
0f62676
to
3696bcf
Compare
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
e2c15bf
to
122e04c
Compare
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 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 |
@marcandre I wanted to collect the If you want me to try again I will but if it is a reasonable solution as-is, that’s ok by me 😅 |
Oh, sorry, I didn't understand completely. I'm surprised you got clobbering errors, as I imagine your corrections were a bunch of The main difference for the user is where to put In short: I'm 👍 either way. If you prefer the multiple offense route and run into difficulties with clobbering, I can have a look. |
@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. |
Oh and the other problem was removing the empty |
Right, I imagine it can be a bit tricky.
"swallowed deletions" are ignored so should not have been an issue to delete it on the last offense.
We're all glad it works, that's the most important! Thanks for this 👍 |
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! |
I found quite a few issues with autocorrection in
Style/ClassMethodsDefinitions
, that are now handled, with fuller test coverage.class << self
block is now handled correctly (previously it would duplicate methods)self << class
blocks after autocorrection are now removed.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
inCommentsHelp
was returning the entire body of thesclass
, 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 thesclass
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:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.