-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[RFC / wip]: Add option to surround uncorrectable offenses with # rubocop:disable
#4127
Conversation
I agree that the TODO is a bit crude, and the biggest shortcoming is if one is frivolously generated and not acted on, it can lead to even more offences silently creeping in. I don't think using My point is:
For me, it is the opposite. If I see a How about an alias directive, that works the same way, but that offers some context? E.g. |
Interesting! I hadn't thought about those semantics, but you make a good point. I really like the idea of having making a |
I really like this idea and have thought about trying to implement it, but never got around to trying anything out in the RuboCop code base. I implemented this in a crude manner on a project several years ago and it went well. If I remember correctly, I ran RuboCop with json output, then parsed the json for line numbers and added the comment based on that.
This was my mentality as well. It was a bigger deal when RuboCop didn't support auto-correct, and I think it is still important since several cops do not support auto-correct. It is a great solution when team agree that they are will to live with current offenses, but they do not want to introduce new offenses to their code base.
This sounds like a great idea to me. |
lib/rubocop/cop/autocorrect_logic.rb
Outdated
begin_line = location.source_buffer.source_line(begin_row) | ||
leading_whitespace = begin_line[/^\s*/] | ||
|
||
corrector.insert_before(range_by_lines, "#{leading_whitespace}# rubocop:disable #{name}\n") |
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.
Should this place comments before and after the offense or just use a single disable?
# rubocop:disable Style/HashSyntax
{ :a => 1 }
# rubocop:enable Style/HashSyntax
{ :a => 1 } # rubocop:disable Style/HashSyntax
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.
That's a good point--I actually didn't know about the single-line disable when I wrote this. :) The feature should probably use single-line disables whenever possible.
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.
Actually @rrosenblum after spelunking through the codebase a bit more I learned that a # rubocop:disable
works for any offense--for multiline offenses it just has to occur at the end of the first line.
So maybe all disables from this feature should be single-line disables?
We'd need to figure out the semantics of how Some options that occur to me:
Option 1 seems less surprising in my opinion. The implementation would be more complex, but it seems worth it to me. Thoughts, anyone? |
85ffcbb
to
82785db
Compare
FYI, I've amended the latest commit to clean up the part that disables offenses. (It also now adds one-line disables for one-line offenses.) I've got some changes WIP to implement Option 1 for the |
82785db
to
91418a7
Compare
91418a7
to
784980f
Compare
See [[1]] for original proposal. [1]: rubocop#4127 (comment)
@jonas054 Any thoughts on how to proceed with this? I want us to clean up stale PRs soon. |
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.
@vergenzt If you can finish the job, I'm all for it.
expect(subject).to eq(0) | ||
expect($stderr.string).to eq('') | ||
expect($stdout.string) | ||
.to eq("#{abs('example.rb')}:1:5: C: [Corrected] Rename `is_example` " \ |
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.
I would prefer if it said Disabled
instead of Corrected
.
.to eq("#{abs('example.rb')}:1:5: C: [Corrected] Rename `is_example` " \ | ||
"to `example?`.\n") | ||
expect(IO.readlines('example.rb').map(&:chomp)) | ||
.to eq(['def is_example # rubocop:disable Style/PredicateName', |
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.
I hope you can implement some form of rubocop:todo
as per the discussion.
Hey all! Sorry for the lack of activity on this. I made consistent progress for a while a few months ago, but the scope of the project ballooned considerably due to some further-reaching-than-intended effects (mostly from picking Option 1 from #4127 (comment); it broke the Lint/UnneededDisable algorithm). I've got most of my thoughts/plans written out in a Google Doc, but it could use some cleaning up/fleshing out. I did some of that this morning but haven't quite finished. Will try to post that doc soon. |
I've made solid progress on this over the last week or so. :D (Thanks for the nudge!) TL;DR: The changes in #4355 would break Details and a full explanation of everything going on are here: Rubocop Comment Directive Architecture. Since the diff is pretty large I'm planning to file it as multiple independent (and independently mergeable) PRs. Will update here when I've got those filed. Basic outline of the plan:
It'll be a whirlwind, but we can do it!!! 🙂 Sound like a plan to everyone? (Sorry this kindof blew up in scope. I hope the complexity is okay. It's been fun to tackle!) |
What's the state here? I'm officially appointing @jonas054 to drive this to finish, but I really would like us to eventually finish this task. :-) |
Oh man me too... So a lot of what happened is (1) I'm no longer using Ruby day-to-day at work (moved teams), and (2) I started an online master's in addition to working full time, so I haven't had much free time at all to devote to personal projects. :( I can take a look at the branches I've got later though and make sure to push what I have if someone else wants to carry the baton to the finish line! IIRC most if it is pretty close to working (I definitely remember that I finished implementing the proposed CommentConfig algorithm and it passed tests), so if anyone wants to learn a ton about Rubocop's architecture and have fun while contributing to an awesome and useful feature let me know. 😄 |
I'll close this for now, as it seems it's not going anywhere. @jonas054 Feel free to continue it if you want down the road. |
I have picked this up at long last, and a PR is forthcoming. |
Hi all. Today I had an idea that rethinks the workflow surrounding the
--auto-gen-config
option, and I wanted to see what you all think.Note: I created this as a PR because I made a proof-of-concept for myself this morning and I thought I'd share that. However since the PR clearly is not ready to be merged and this is more of an RFC, if you'd prefer this be an issue I'd be happy to close and reopen.
Situation
I maintain a large-ish Rails app (~16k nonblank lines of Ruby code) which has been in development for ~2 years now with 3 different primary maintainers. Rubocop was just recently introduced, so there's a lot of inconsistently-styled code lying around.
Current best practice
If I understand correctly the current recommendation for introducing Rubocop to a legacy codebase is to use
--auto-gen-todo
to create a.rubocop_todo.yml
file with your "todo list" that you chip away at over time.I did this a few months ago, but the solution hasn't been especially satisfying to me, for a few reasons:
You have to decide to visit
.rubocop_todo.yml
and actively search for something to fix if you're in the mood, and while perusing it you have to be able to quickly understand what the various cop names refer to.The format of
.rubocop_todo.yml
makes the Boy Scout rule difficult to enforce. Since the todo list's finest level of granularity is the file level, once a file violates a given cop anywhere (and that violation gets the file added to.rubocop_todo.yml
's exclude list), it's easy to just keep on adding violations in new code instead of fixing things as you go.Proposed alternative
What if Rubocop had an option to add
# rubocop:disable
to all current violations rather than adding them to a todo list? This would make it much easier to see styleguide violations present in the section of code being edited, and would make the process for fixing them obvious.For example if I'm editing method
foo
and I see it's surrounded by# rubocop:disable Metrics/MethodLength
, then that's a lot more likely to prod me to refactor the method than ifMetrics/MethodLength
happened to include the file in its exclude list in a yaml todo list file I never think to check. (I wouldn't even know from the current todo list format which method it is that's too long unless I go through the trouble to look up the metric's default max value! That's a lot of work for something that's supposed to happen frequently--i.e. leaving a section of code cleaner than you found it.)Proof of concept
My thought was that
--auto-correct
could have a companion flag--disable-uncorrectable
which would add a properly-indented# rubocop:disable #{cop_name}
line before the violation and# rubocop:enable #{cop_name}
after the violation, for cops that do not support an autocorrection of their own.What do you think?