-
Notifications
You must be signed in to change notification settings - Fork 1.6k
parser: parsing doublestar as a power op without a left hand side #21720
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
Signed-off-by: 11happy <[email protected]>
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| ERA001 | 1 | 1 | 0 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+1 -0 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)
apache/superset (+1 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL
+ superset/config.py:262:1: ERA001 Found commented-out code
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| ERA001 | 1 | 1 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
MichaReiser
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.
Would you mind adding a few parser tests that demonstrate that the new recovery logic works as expected?
E.g., the ecosystem changes suggest that we:
a) Fail to emit a diagnostic in that case
b) or we now use this code path for **kwargs when we shouldn't
Signed-off-by: 11happy <[email protected]>
I have added tests & I think its not the option b, I would appreciate any pointers to move forward |
MichaReiser
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.
Would you mind changing your PR to use the testing infrastructure described here
| ### Inline tests |
Can you also add an example where there's some content to the right, e.g. ** a + 3 * 2
| TokenKind::DoubleStar => { | ||
| self.bump(TokenKind::DoubleStar); | ||
| let right = | ||
| self.parse_binary_expression_or_higher(OperatorPrecedence::Exponent, context); | ||
| return Expr::BinOp(ast::ExprBinOp { | ||
| left: Box::new(Expr::Name(ast::ExprName { | ||
| range: self.missing_node_range(), | ||
| id: Name::empty(), | ||
| ctx: ExprContext::Invalid, | ||
| node_index: AtomicNodeIndex::NONE, | ||
| })), | ||
| op: Operator::Pow, | ||
| right: Box::new(right.expr), | ||
| range: self.node_range(start), | ||
| node_index: AtomicNodeIndex::NONE, | ||
| }) | ||
| .into(); |
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 not sure if this is the right place for the fix. Would it be possible to add it to parse_binary_expression_or_higher or even in parse_binary_expression_or_higher_recursive? If not, can you explain why?
Moving it their would give you the diagnostic for free
Summary
Parses
**Doublestar as power op without lhs, Part of #10653, referred discussion #10636 (comment)Test Plan
test case :
{x: **y for x, y in data}ruff-dev output before:
ruff-dev output ast after: