Consolidate ExprYield/ExprYieldFrom into AnyExpressionYield
#6121
Conversation
| use ruff_formatter::formatter::Formatter; | ||
| use ruff_formatter::prelude::{space, text}; | ||
| use ruff_formatter::{write, Buffer, FormatResult}; | ||
| use ruff_formatter::{Format, FormatResult}; | ||
| use ruff_python_ast::node::AnyNodeRef; | ||
| use ruff_python_ast::ExprYield; | ||
| use ruff_text_size::TextRange; |
There was a problem hiding this comment.
Nit: You should get Formatter, Format, FormatResult, and all the inports from ruff_formatter::preludeautomatically by usingcrate::prelude::*` (Meaning, we could remove those)
| Expr::Yield(expr) => AnyExpressionYield::from(expr).needs_parentheses(parent, context), | ||
| Expr::YieldFrom(expr) => { | ||
| AnyExpressionYield::from(expr).needs_parentheses(parent, context) | ||
| } |
There was a problem hiding this comment.
Nit: I would keep the impl NeedsParentheses for ExprYield and ExprYieldFrom so that callers (including the call here) don't need to know that the implementation is shared by calling into AnyExpressionYield::from(expr).needs_parentheses
|
Hey @qdegraaf — sorry about this but we needed to remove a commit from the main branch so I've changed the base of your branch to fix the history. Please let me know if you have any questions or problems! |
Hey @zanieb. Thanks for the heads up. I got too trigger happy anyway and refreshed my fork forgetting I had a couple other PRs open as well. Reopened this one in: #6127 FYI @konstin @MichaReiser due to previous reviews |
Summary
Removes some duplicated logic by consolidating both
yieldexpressions into one enum, as suggested by @MichaReiser in #5921 (review)Test Plan
Existing tests and functionality were checked to be unchanged, this is a pure refactor PR