Add ProcessedSource#tokens_within, ProcessedSource#first_token_of and ProcessedSource#last_token_of
#92
Conversation
| end | ||
|
|
||
| it 'returns tokens for heredoc node' do | ||
| node = ast.children[0].arguments[2] |
There was a problem hiding this comment.
Aren't the heredoc's tokens a bigger issue for #{} within the heredoc?
There was a problem hiding this comment.
Didn't get the point. Can you reword/explain more?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
fatkodima
force-pushed
the
node-tokens
branch
2 times, most recently
from
a9209a8 to
60e2c10
Compare
lib/rubocop/ast/processed_source.rb
Outdated
| end | ||
|
|
||
| def last_token_index(source_range) | ||
| sorted_tokens.bsearch_index { |token| token.end_pos >= source_range.end_pos } |
There was a problem hiding this comment.
How about begin_pos = source_range.begin_pos before the bsearch_index, as a "free" optimization, or passing that instead of source_range?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/rubocop/ast/processed_source.rb
Outdated
| @@ -159,6 +159,20 @@ def line_indentation(line_number) | |||
| .length | |||
| end | |||
|
|
|||
| def tokens_within(source_range) | |||
There was a problem hiding this comment.
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?
lib/rubocop/ast/processed_source.rb
Outdated
| sorted_tokens[begin_index..end_index] | ||
| end | ||
|
|
||
| def first_token_of(source_range) |
There was a problem hiding this comment.
We could accept node or source_range here too and in last_token_of
|
Updated to accept |
lib/rubocop/ast/processed_source.rb
Outdated
| end | ||
|
|
||
| def source_range(range_or_node) | ||
| if range_or_node.is_a?(RuboCop::AST::Node) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
ok, made it a duck 😄
|
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. |
… and `ProcessedSource#last_token_of`
fatkodima
changed the title
ProcessedSource to get info about tokens of concrete nodesProcessedSource#tokens_within, ProcessedSource#first_token_of and ProcessedSource#last_token_of
Looks like a startup idea 😄 Something like |
|
Super, thanks! 🎉 |
Context - rubocop/rubocop#8450
That was merged a bit fast 😄 - will send PR with refactorings when this will be released.