-
Notifications
You must be signed in to change notification settings - Fork 196
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
implement PEP-654: except* #571
Conversation
@stroxler xmas treat for you :) |
Codecov Report
@@ Coverage Diff @@
## main #571 +/- ##
==========================================
+ Coverage 94.62% 94.65% +0.03%
==========================================
Files 240 240
Lines 23389 23579 +190
==========================================
+ Hits 22132 22319 +187
- Misses 1257 1260 +3
Continue to review full report at Codecov.
|
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.
LGTM, I left a couple comments.
The big thing I skipped was the matcher code, I can take a look at that but it will require some self-training so I figured I'd let you decide whether it's worth it first.
@@ -498,6 +499,13 @@ parser! { | |||
make_try(kw, b, ex, el, f) | |||
} | |||
|
|||
// Note: this is separate because TryStar is a different type in LibCST | |||
rule try_star_stmt() -> TryStar<'a> |
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 official grammar has this as a variant under try_stmt
. We're sure it's worth diverging? I can see some value in it since the actual AST node variant is different, but there's a price we'll pay keeping things in sync for every rule that diverges.
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.
Not 100% sure it's worth it, I was a bit lazy and wanted to avoid an additional enum layer just so we can have a single rule.
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.
Oh are the rules tied more tightly to types in the rust PEG library than in C?
In CPython it's a single PEG variant, but two different AST types still
Summary
This PR adds support for the new grouped exception handling mechanism introduced by PEP-654. The only significant difference between AST and this CST implementation is a new
ExceptStarHandler
type, which differs fromExceptHandler
in:type
attribute to be always present (it can't beNone
)except
keyword and the*
The new syntax can only be parsed by the Rust parser (currently needing
LIBCST_PARSER_TYPE=native
environment variable).Test Plan