-
Notifications
You must be signed in to change notification settings - Fork 154
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
Aliases for Ripper Translation #2423
Conversation
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.
A couple of small changes, but overall looks good.
@@ -220,7 +220,7 @@ module Prism | |||
source, filepath = read_source(argv) | |||
|
|||
ripper = Ripper.sexp_raw(source) | |||
prism = Prism::RipperCompat.sexp_raw(source) | |||
prism = Prism::Translation::Ripper.sexp_raw(source) rescue :parse_error |
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 specifically rescue NotImplementedError
(I'm assuming that's what your handling)? I wouldn't want this to hide a different problem.
prism = Prism::Translation::Ripper.sexp_raw(source) rescue :parse_error | |
prism = | |
begin | |
Prism::Translation::Ripper.sexp_raw(source) | |
rescue NotImplementedError | |
:parse_error | |
end |
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.
I mostly use it to make sure that if something's not working yet in Translation::Ripper that I still see CRuby's Ripper output. When it says "parse_error" that should only hide another problem if I'm expecting a parse error. But if I'm midway through making a modification and it might get an error, this means that "prism ripper" is still useful. I guess I could add a mode that doesn't actually use Prism's Ripper and just prints out CRuby's output without trying Prism?
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.
Ahh that's fine. We can keep it for now, I just want this to go away eventually.
# no block call. It can be hard to tell which of multiple equivalent | ||
# structures it will produce. This method attempts to return a normalized | ||
# comparable structure. | ||
def normalized_sexp(parsed) |
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.
I'm fine having this for now, but we'll definitely want to get rid of this before we close this ticket out.
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.
I'll definitely try. I'm having trouble figuring out when Ripper does and doesn't emit this, even looking through parse.y.
Found a minor TruffleRuby Ripper incompatibility. Turning off that test on Truffle, filed a bug: oracle/truffleruby#3457 |
Turning off the command line test on Windows. Aside from issues like not being able to just run bin/prism (need "ruby bin/prism" or rough equivalent) there's also not a great "which" equivalent. |
Yeah, I'm just removing the command line test. That's annoying, I wanted to add it because the utility broke. But I'm not figuring out how to locate bin/prism to run it reliably, even if I exclude Windows. I can find it in dev checkouts (__dir__ + ../../bin/prism) and in installed gems ( |
@@ -220,7 +220,7 @@ module Prism | |||
source, filepath = read_source(argv) | |||
|
|||
ripper = Ripper.sexp_raw(source) | |||
prism = Prism::RipperCompat.sexp_raw(source) | |||
prism = Prism::Translation::Ripper.sexp_raw(source) rescue :parse_error |
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.
Ahh that's fine. We can keep it for now, I just want this to go away eventually.
Handle more aliases, including fully handling aliases.txt.
As part of handling more aliases, handle symbols that are keywords, constants or operations. Handle interpolated symbols. Handle global variable references.
Better testing of prism ripper CLI and a test for it. I had broken it in renaming Translation::Ripper. Now it's tested.It also prints better output when Prism or Ripper Translation gets an exception during parsing.