-
Notifications
You must be signed in to change notification settings - Fork 228
Use prism to parse comments #2828
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
Conversation
|
Nice. Incidentally today I was trying the UPDATE: seems it's not Ripper related, basically same before & after this PR: |
lib/rbs/prototype/rb.rb
Outdated
| else | ||
| hash[line] = comment | ||
| end | ||
| comments = Prism.parse(string).yield_self do |parse_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.
Should use version: "current", right?
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.
Ah, yeah. I kept ripper around for ruby 3.2 since prism can't parse 3.2 syntax.
Since the two implementations for RB and RBI are pretty much identical except for trailing comments I consolidated their implementations so that there is not so much duplication.
The long-term goal is to deprecate ripper: https://bugs.ruby-lang.org/issues/21827 So, this starts using prism to parse. Prism already knows if a comment is preceeded by code via `trailing?`, so that makes the `RB` case a bit simpler.
c67fe7e to
67563ca
Compare
Earlopain
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.
The rbi of prism is not correct, missing the version option. I created ruby/prism#3874 to fix this, anything I can do here to work around it for now?
| spec.required_ruby_version = ">= 3.2" | ||
| spec.add_dependency "logger" | ||
| spec.add_dependency "prism", ">= 1.3.0" | ||
| spec.add_dependency "prism", ">= 1.6.0" |
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.
This is the prism version that added support for Prism.parse(version: "current"). Basically it is Prism.parse(version: RUBY_VERSION) with a nicer error message when the prism gem is outdated.
|
As a workaround, you can add |
The long-term goal is to deprecate ripper: https://bugs.ruby-lang.org/issues/21827 So, this starts using prism to parse. Prism already knows if a comment is preceeded by code via `trailing?`, so that makes the `RB` case a bit simpler. Ripper is still used when running as ruby 3.2 because prism can't parse 3.2 syntax. When runtime support for 3.2 is dropped, the fallback code can be dropped as well.
|
Thanks! I fixed the typecheck by applying your suggestion. The next prism release will contain correct signatures, so it can be removed once that happens. |
| # Prism can't parse Ruby 3.2 code | ||
| if RUBY_VERSION >= "3.3" |
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.
Could use Prism for all Ruby versions if we're OK to parse 3.2 code with Prism 3.3 syntax, which should be fine for comments: #348 (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.
Honestly, seems fine to keep around for now. The ripper code has already been written and can just be removed at a later time.
soutaro
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.
Thanks!
The long-term goal is to deprecate ripper: https://bugs.ruby-lang.org/issues/21827
So, this starts using prism to parse. Prism already knows if a comment is preceeded by code via
trailing?, so that makes theRBcase a bit simpler.