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

Document the current state of the project in the readme #1069

Merged
merged 1 commit into from
Mar 14, 2025

Conversation

Earlopain
Copy link
Contributor

Because of #1046. It will look like this:

Warning

The parser gem can only correctly parse Ruby code up to and including Ruby 3.3. For Ruby 3.4 and later, please use the Prism translation parser instead.
Starting in Ruby 3.4, Prism is the parser used in Ruby itself and can produces AST that is identical to the output of parser. If you only need to parse Ruby 3.3 (or greater), also consider using the native Prism AST.
See this GitHub issue for more details.

I think this makes sense?

@iliabylich
Copy link
Collaborator

Yes, LGTM 👍

README.md Outdated
@@ -13,6 +13,11 @@ equivalent source code from Parser's ASTs.
Sponsored by [Evil Martians](http://evilmartians.com).
MacRuby and RubyMotion support sponsored by [CodeClimate](http://codeclimate.com).

> [!WARNING]
> The `parser` gem can only correctly parse Ruby code up to and including Ruby 3.3. For Ruby 3.4 and later, please use the [Prism translation parser](https://github.com/ruby/prism/blob/main/docs/parser_translation.md) instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's just my two cents.

Suggested change
> The `parser` gem can only correctly parse Ruby code up to and including Ruby 3.3. For Ruby 3.4 and later, please use the [Prism translation parser](https://github.com/ruby/prism/blob/main/docs/parser_translation.md) instead.
> The `parser` gem is only compatible parse Ruby code up to and including Ruby 3.3. For Ruby 3.4 and later, please use the [`Prism::Translation::Parser`](https://github.com/ruby/prism/blob/main/docs/parser_translation.md) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tweaked the first part a bit since a word was missing otherwise

README.md Outdated
@@ -13,6 +13,11 @@ equivalent source code from Parser's ASTs.
Sponsored by [Evil Martians](http://evilmartians.com).
MacRuby and RubyMotion support sponsored by [CodeClimate](http://codeclimate.com).

> [!WARNING]
> The `parser` gem can only correctly parse Ruby code up to and including Ruby 3.3. For Ruby 3.4 and later, please use the [Prism translation parser](https://github.com/ruby/prism/blob/main/docs/parser_translation.md) instead.
> Starting in Ruby 3.4, Prism is the parser used in Ruby itself and can produces AST that is identical to the output of `parser`. If you only need to parse Ruby 3.3 (or greater), also consider using the native Prism AST.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be helpful to add a note clarifying the differences in compatibility between the respective ASTs.

Suggested change
> Starting in Ruby 3.4, Prism is the parser used in Ruby itself and can produces AST that is identical to the output of `parser`. If you only need to parse Ruby 3.3 (or greater), also consider using the native Prism AST.
> Starting in Ruby 3.4, Prism is the parser used in Ruby itself and can produces AST that is identical to the output of `parser`. If you only need to parse Ruby 3.3 (or greater) and don't require compatibility with the `parser` gem AST, also consider using the native Prism AST.

Verified

This commit was signed with the committer’s verified signature.
vszakats Viktor Szakats
@iliabylich
Copy link
Collaborator

Let's merge it as is and tweak later?

@koic
Copy link
Collaborator

koic commented Mar 14, 2025

Yeah, let's proceed with that.

@iliabylich iliabylich merged commit 971e279 into whitequark:master Mar 14, 2025
10 checks passed
@iliabylich
Copy link
Collaborator

Thanks guys!

@BuonOmo
Copy link

BuonOmo commented Mar 20, 2025

Hey! I'm a bit late, but could we consider having some migration guide for users of the rewriter api ? For instance in my codebase I basically had to change from:

require "parser/current"

ast = Parser::CurrentRuby.parse_file(...)
buffer = Parser::Source::Buffer.new(...)
class ChangeSomething < Parser::TreeRewriter
end

to:

require "parser"
require "prism"

ast = Prism::Translation::Parser.parse_file(...)
buffer = Parser::Source::Buffer.new(...)
class ChangeSomething < Parser::TreeRewriter
end

Source: cockroachdb/activerecord-cockroachdb-adapter#370

(at least I believe that is the way of proceeding? ^^)

@Earlopain
Copy link
Contributor Author

Earlopain commented Mar 21, 2025

Yes, that PR looks ok to me. One note thought, that Prism::Translation::Parser is not equivalent to Parser::CurrentRuby because it always parses as Ruby 3.4. For the tests, I don't believe that is relevant but I wanted to mention it. You can refer to it via Prism::Translation::Parser34 to make that clear.

I wrote #1072 which should cover the most relevant cases. Just recently I also added something like CurrentRuby to prism, once that is released I will update the guide to mention that.

And yeah, unfortunately no rewriting with native prism for now as far as I'm aware ruby/prism#3466

@BuonOmo
Copy link

BuonOmo commented Mar 21, 2025

So maybe we could add a reference to this doc in the warning message, and add a word about rewriting? I could do it next week :)

@Earlopain
Copy link
Contributor Author

Sure, I think it could make sense to add a more detailed warning message specific to 3.4. I'd add a header to the warning in the readme so its linkable and put that in the warning somewhere. Feel free to open a PR.

@Earlopain
Copy link
Contributor Author

Also, I haven't looked much into rewriting yet and am not even sure how it works for parser. But I think it would be nice if you could mention in the prism issue that you need that in order to fully switch to prism

@BuonOmo
Copy link

BuonOmo commented Mar 21, 2025

the prism issue

Is it #1046 ? I already saw that people mentionned it, with great ideas (a separate gem for rewriting for instance). I 👍ed the messages, and didn't feel like we'd need one more message 😅

@Earlopain
Copy link
Contributor Author

Nah, I meant this one ruby/prism#3466, I linked it above.

I'm somewhat sure that the prism maintainer is interested in that as a first-party functionality. I think that has come in the issue you linked sometime.

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

4 participants