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

Review comments API #83

Merged
merged 3 commits into from Aug 1, 2020
Merged

Review comments API #83

merged 3 commits into from Aug 1, 2020

Conversation

marcandre
Copy link
Contributor

This PR:

  • Adds ProcessedSource#comment_at_line
  • Adds ProcessedSource#each_comment_in_lines

It deprecates ProcessedSource#comments_before_line (which is imo a big code smell).

I can't believe how much time I spent to arrive to this quite small PR; it's like version 5.0, I was really slow to get to this API.

I have reviewed / fixed all uses of any comment-accessing method in the main gem. The PR is upcomming. My main take is that any PR that autocorrects any range that could span more than one line must have specs with comments on each single line (close to that range).

@marcandre
Copy link
Contributor Author

@bbatsov, do you prefer I wait for your approval for this kind of PR?

@bbatsov
Copy link
Contributor

bbatsov commented Jul 30, 2020

Sorry about the slow response. I was on vacation for a week and I'm behind on a ton of things. The changes seem fine to me.

@bbatsov, do you prefer I wait for your approval for this kind of PR?

If you're very confident about some changes you don't really need my approval. I don't want to be a bottleneck for progress. :-)

@@ -99,22 +99,39 @@ def blank?
ast.nil?
end

# @return [Comment, nil] the comment at that line, if any.
def comment_at_line(line)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this covers both whole line comments and inline comments, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! API that applies only to whole line comments should be pretty rare imo

@marcandre
Copy link
Contributor Author

I was on vacation for a week

Great, hope you had a good time!

@marcandre
Copy link
Contributor Author

If you're very confident about some changes you don't really need my approval.

Only issue with this is I'm a very confident person 😅, and I don't always put enough emphasis on naming / doc.

Given these, I'll merge #82 which I consider obvious, and I'll wait for #72 which is on hold because of the name of a module meant for internal use (your input appreciated)...

@marcandre marcandre merged commit add3be7 into rubocop:master Aug 1, 2020
@marcandre marcandre deleted the no_comment branch August 1, 2020 18:57
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