-
Notifications
You must be signed in to change notification settings - Fork 141
Nesting analysis using Prism #1092
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
base: master
Are you sure you want to change the base?
Conversation
4c8c4f2 to
be7dfe6
Compare
be7dfe6 to
c3a31b8
Compare
st0012
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we nudge Prism maintainers to cut v1.8.1 for the fix, and add prism as a dependency and require 1.3.0+?
| # scan_opens without block will return a list of open tokens at last token position | ||
| scan_opens(tokens) | ||
| # Return a list of open nestings at last token position | ||
| def open_nestings(parse_lex_result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this or parse_by_line take (code, local_variables: []) and call Prism.parse_lex here?
I feel Prism should be hidden from the callers of this class. Then we only need to require it here instead of all the places that'd use a nesting parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a followup pull request #1160, parse_lex_result will be also used in RubyLex#should_continue?.
To avoid parsing the same code two times in dynamic_prompt calculation, argument needs to be parse_lex_result.
@context.io.dynamic_prompt do |lines|
parse_lex_result = Prism.parse_lex(code, scopes: [@context.local_variables])
line_results = IRB::NestingParser.parse_by_line(parse_lex_result)
...
line_results.map.with_index do |...|
# This part requires lex result
continue = @scanner.should_continue?(tokens_until_line, line, line_num_offset + 1)
...
end
endMaybe we can find a better way to separate after migrating to Prism, I think.
|
Oh you did this in #1091 already. I guess we just need to wait for the release then |
#1024
Migrate nesting analysis from Ripper to Prism
Original nesting calculation with Ripper
Tokenize with ripper
Parse the tokens mainly focusing on open/close tokens
Collect open tokens per line
New nesting calculation with Prism
Traverse syntax tree
Check open_loc and closing_loc for a node that makes nesting, create a token-like object for it
Collect open token-like objects per line
Gemfile
Exclude
prism-1.8.0because test fails. (ruby/prism#3851)