-
Notifications
You must be signed in to change notification settings - Fork 174
Add Ripper :on_sp events for Prism.lex_compat and Prism::Translation::Ripper #3859
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: main
Are you sure you want to change the base?
Conversation
|
Failures that were added to excludes: |
|
And more complex cases like Would there be any way to get the correct state with Prism? |
|
I would not bother with comparing state for this. I'm not particularly motivated to understand what is happening in detail here. I doubt anyone is relying on this information being correct. |
|
Yeah it's what I'm doing in test/prism/ruby/ripper_test.rb, I'm ignoring the state of Slightly related, is |
Just putting in the state from the previous token seems fine. When it lines up, great. If not, oh well. Doesn't really matter in what way it's wrong.
I believe that is what ripper itself does. I haven't benchmarked this in detail yet but there are definitly some hotspots. Much time is spend sorting tokens by location for example. If changing this makes a noticable difference, I wouldn't be opposed to it. Here's what I use to quickly check: require "ripper"
require "prism"
require "benchmark/ips"
codes = Dir["**/*.rb"].map { File.read(it) }
Benchmark.ips do |x|
x.report("prism") { codes.each { Prism::Translation::Ripper.lex(it) } }
x.report("ripper") { codes.each { Ripper.lex(it) } }
x.compare!
endI was also a bit concerned about the performance of this implementation but it is not so bad. Compared to ripper it goes from 2.3x times slower to 2.6x times slower which seems fine. |
Ref: #3838
On top of #3858 should be merged first.