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 ProcessedSource#commented? for multi-line ranges. Add ProcessedSource#commented_line? #55

Merged
merged 2 commits into from Jul 12, 2020

Conversation

marcandre
Copy link
Contributor

ProcessedSource#commented? was only considering the beginning line of the given range

@marcandre marcandre force-pushed the comment branch 2 times, most recently from 86bf60c to e3f08dc Compare July 5, 2020 02:42
@@ -95,8 +95,16 @@ def blank?
ast.nil?
end

# @return [Boolean] if the given line number has a comment.
def commented_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 think the name is a bit misleading (it sounds like a line that was commented out), but it's a good change. I guess you just tried to mimic the commented? name we've employed before. Perhaps it should have been something like contains_comments? or whatever.

Copy link
Contributor Author

@marcandre marcandre Jul 6, 2020

Choose a reason for hiding this comment

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

I agree, I should have thought of a better name, especially that util.rb in rubocop has comment_line? which is true only for full lines.
So which is best: contains_comment?, or line_with_comment?.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can make a method contains_comment? that acts differently on line numbers and source ranges. 😉 If not - I'd go with line_with_comment?, as I suggested contains_comment? as an alternative name for commented?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with line_with_comment? and contains_comment? (with deprecated alias commented?)

@marcandre marcandre force-pushed the comment branch 2 times, most recently from d31430f to dc52ee7 Compare July 9, 2020 05:12
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