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

Refactor comment access #8405

Merged
merged 9 commits into from Aug 10, 2020
Merged

Refactor comment access #8405

merged 9 commits into from Aug 10, 2020

Conversation

marcandre
Copy link
Contributor

This PR fixes all occurrences of ProcessedSource#find_comment, each_comment, comments_before_line, commented? and improves processing in general.

Fixes two cops with bad comment treatment: Style/EmptyCaseCondition, SaveNavigation. I didn't check other cops, is just that the processing for these was clearly erroneous.

util.rb's comment_line? remains though, even though it is a bad API (it should probably be something like ProcessedSource#line_is_comment?(line_nb) instead).

This PR relies on an unreleased version of rubocop-ast

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 5, 2020

Apart from the failing build, the changes look good to me.

@marcandre
Copy link
Contributor Author

Right, now that rubocop-ast is released I can update this and should be green

@marcandre marcandre force-pushed the no_comment branch 3 times, most recently from d723d10 to f3b2848 Compare August 5, 2020 20:36
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 8, 2020

Seems you'll also have to rebase.

@marcandre
Copy link
Contributor Author

I wish there was a way to flag a PR as "auto merge"; if the CI is green merge instantly... I wait for the CI and then forget about it 😅
Should I wait after the next point release to merge?

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 8, 2020

That PR affects only the internals, so I think we can merge it right away.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 8, 2020

I wish there was a way to flag a PR as "auto merge"; if the CI is green merge instantly...

Yeah, that'd be a cool feature indeed!

Highlight for style/safe_navigation_spec isn't perfect though
Avoids incorrect use of `comments_before_line`.
The autocorrection worked only for case statements that were root of the document and were not indented
Also fix `Style/SafeNavigation`'s auto-correction, typical off-by-one error without a spec
@bbatsov bbatsov merged commit e0f28e0 into rubocop:master Aug 10, 2020
@marcandre
Copy link
Contributor Author

I wish there was a way to flag a PR as "auto merge"; if the CI is green merge instantly...

Yeah, that'd be a cool feature indeed!

Could we consider https://github.com/marketplace/mergify ?

There are other bots available as well as github actions, but [some people] had good success with mergify.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 11, 2020

mergify looks nice. We can certainly try it.

koic added a commit that referenced this pull request Sep 18, 2022
ydah added a commit to ydah/rubocop-rspec that referenced this pull request Feb 7, 2023
`RuboCop::Cop::Util#comment_line?` is flagged by rubocop/rubocop#8405 as a bad API.
So I propose to fix not use this method.
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.

None yet

2 participants