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

Add ProcessedSource#tokens_within, ProcessedSource#first_token_of and ProcessedSource#last_token_of #92

Merged
merged 1 commit into from Aug 6, 2020

Conversation

fatkodima
Copy link
Contributor

Context - rubocop/rubocop#8450

That was merged a bit fast 😄 - will send PR with refactorings when this will be released.

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Good PR, but I have a few recommendations on the API

lib/rubocop/ast/processed_source.rb Outdated Show resolved Hide resolved
lib/rubocop/ast/processed_source.rb Outdated Show resolved Hide resolved
lib/rubocop/ast/processed_source.rb Outdated Show resolved Hide resolved
lib/rubocop/ast/processed_source.rb Outdated Show resolved Hide resolved
end

it 'returns tokens for heredoc node' do
node = ast.children[0].arguments[2]
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't the heredoc's tokens a bigger issue for #{} within the heredoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't get the point. Can you reword/explain more?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering: would your test fail if the tokens were not sorted? I thought it was for cases where there's a heredoc with for #{} that doing the bsearch on unsorted tokens might fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When tokens are not sorted, for some cases, depending on the source, it will fail, sometimes not. It depends on how lucky bsearch will be to find correct locations.
When they are sorted, it will always succeed.

lib/rubocop/ast/processed_source.rb Outdated Show resolved Hide resolved
lib/rubocop/ast/processed_source.rb Outdated Show resolved Hide resolved
@fatkodima fatkodima force-pushed the node-tokens branch 2 times, most recently from a9209a8 to 60e2c10 Compare August 6, 2020 08:30
Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Wow, sooo clean now 👍

end

def last_token_index(source_range)
sorted_tokens.bsearch_index { |token| token.end_pos >= source_range.end_pos }
Copy link
Contributor

Choose a reason for hiding this comment

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

How about begin_pos = source_range.begin_pos before the bsearch_index, as a "free" optimization, or passing that instead of source_range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can extract it into local variable, but it seems that won't give much a difference. For linear search this probably will have noticeable effect, but for bsearch - not sure.

I think, most of the time this method will be used with Nodes as an argument, so 3 variants of arguments looks equal to me. source_range looks more clear to me, but I have no strong opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel supporting source_range is important as it is the most generic. There might not be a Node with the source_range someone needs.

most of the time this method will be used with Nodes

So let's accept a source_range, or anything that responds_to?(:source_range) then.

@@ -159,6 +159,20 @@ def line_indentation(line_number)
.length
end

def tokens_within(source_range)
Copy link
Contributor

Choose a reason for hiding this comment

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

Last thing: maybe we should accept a source_range or a node?
I try to avoid these for fast methods, but here a single if won't be noticeable. WDYT?

sorted_tokens[begin_index..end_index]
end

def first_token_of(source_range)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could accept node or source_range here too and in last_token_of

@fatkodima
Copy link
Contributor Author

Updated to accept source_range and Node.

end

def source_range(range_or_node)
if range_or_node.is_a?(RuboCop::AST::Node)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that Comment responded to :source_range but it doesn't. I'll open an issue with parser coz it should I think :-)
Was there a reason to prefer checking for a class instead of duck typing with respond_to? :source_range as I had proposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, made it a duck 😄

@marcandre
Copy link
Contributor

Perfect. I <3 it. We're ready to merge, but ChangeLog entry must be moved. These conflicts in the Changelog are quite annoying. I'd list the 3 methods added in it btw.

@fatkodima fatkodima changed the title Add methods to ProcessedSource to get info about tokens of concrete nodes Add ProcessedSource#tokens_within, ProcessedSource#first_token_of and ProcessedSource#last_token_of Aug 6, 2020
@fatkodima
Copy link
Contributor Author

These conflicts in the Changelog are quite annoying

Looks like a startup idea 😄 Something like dependa_bot, but for changelog entries 😄

@marcandre marcandre merged commit ff710b5 into rubocop:master Aug 6, 2020
@marcandre
Copy link
Contributor

Super, thanks! 🎉

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