Conversation
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
crates/ruff_python_formatter/resources/test/fixtures/black/simple_cases/delete.py
Outdated
Show resolved
Hide resolved
|
|
||
| impl Format<PyFormatContext<'_>> for DeleteList<'_> { | ||
| fn fmt(&self, f: &mut Formatter<PyFormatContext<'_>>) -> FormatResult<()> { | ||
| write!( |
There was a problem hiding this comment.
you shouldn't need a write call here, the JoinBuilder writes internally (see ExprSequence)
There was a problem hiding this comment.
I wanted to ask this earlier, so I apologize, but I spent some time actually reading Python's AST docs to see if I'm conflating concepts.
My brain wants to interpret del Expr* as del ExprSequence. If that's the case, would we want to just reuse your ExprSequence implementation here? Maybe I'm missing something.
| # Completed | ||
| ) |
There was a problem hiding this comment.
I double-checked this. Could be another black bug.
There was a problem hiding this comment.
if you filed black bugs link them in comments, it's really helpful to be able to read up later why certain decisions were made
No attached node
Some differences with black but stable
| for element in self.delete_list { | ||
| join.entry(&format_with(|f| { | ||
| write!(f, [element.format().with_options(Parenthesize::IfBreaks)]) | ||
| })); | ||
| } | ||
| join.finish() |
There was a problem hiding this comment.
f.join_with(...)
.entries(self.delete_list.iter().formatted())
// ...I don't think is what I'm looking for, but I'll look into it more.
There was a problem hiding this comment.
formatted may not work for you because it doesn't allow you to pass any options (at least not to my knowledge)
| # Delete something | ||
| del x, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, b, c, d # Delete these | ||
| # Ready to delete |
| let separator = | ||
| format_with(|f| group(&format_args![text(","), soft_line_break_or_space(),]).fmt(f)); |
There was a problem hiding this comment.
Nit
| let separator = | |
| format_with(|f| group(&format_args![text(","), soft_line_break_or_space(),]).fmt(f)); | |
| let separator = group(&format_args![text(","), soft_line_break_or_space()]); |
There was a problem hiding this comment.
You probably want to wrap the whole list in a group rather than each separator so that all soft_line_breaks become hard_line_breaks when a single element doesn't fit on the line. Or you may not need the group at all. But that depends on the expected formatting.
There was a problem hiding this comment.
let separator = group(&format_args![text(","), soft_line_break_or_space()]);
Hmm. I think I tried this but had temporary value issues. I'll check it out.
You probably want to wrap the whole list in a group rather than each separator
I'll play around with it. Thanks!
There was a problem hiding this comment.
I think #5169 (comment) might resolve this as well
Set output to target
|
I'm going on a trip this week, so I apologize for the less than productive PRs. FWIW even having to chip away at this stuff, I do find myself grasping more and more of I find playing with Hopefully I'll have these wrapped up this or next week. Sorry for the wait! Feel free to supersede me if needed -- I get it. |
|
@cnpryer nothing to worry about. Thanks for working on |
| a, | ||
| ) # Trailing | ||
|
|
||
| del a, a |
There was a problem hiding this comment.
Huh. Actually black will format this as
del (x, x)There was a problem hiding this comment.
Maybe this is because it could be interpreted as del tuple(x, x) instead of del obj, obj?
A more reasonable example would be
a, b, c = 1, 2, 3
del (
a,
b,
c
)But I'd guess that's not actually performing a delete of a, b, and c but instead a new object of tuple(a, b, c)
There was a problem hiding this comment.
Makes sense
>>> print(ast.dump(ast.parse('del (a, b, c)'), indent=4))
Module(
body=[
Delete(
targets=[
Tuple(
elts=[
Name(id='a', ctx=Del()),
Name(id='b', ctx=Del()),
Name(id='c', ctx=Del())],
ctx=Del())])],
type_ignores=[])
|
I want to think about this more before I request a review. |
|
So this looks stable and doesn't seem to introduce syntax errors with what fixtures already exist. I think I could throw test cases at this for days, so instead of going down that hole I'll mark this as ready for a review. At this stage I'd like to play more with my One thing I noticed I have trouble grasping so far is when a comment is "dangling" vs just "trailing". IIUC it has to do with Node assignment? |
|
I can loop around to check this out #5595 (comment) |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
So many comment test cases, nice!. Thank you
3 participants
Summary
Format
deletestatement withParenthesize::IfBreaks.Test Plan
Add basic ruff fixture with comments.
TODO:
DeleteListJoinerjoin_comma_separatedfor many targets