-
Notifications
You must be signed in to change notification settings - Fork 66
reduce the use of trailing commas; add blank lines between groups of fields #390
Conversation
A word of caution: the trailing commas were added for performance reasons, as internal projects were taking 90 seconds in building a single generated mockito files, the additional time attributed to Also see much discussion on the PR in which trailing commas were added: #376 Also, a PR like this which changes output of existing code requires a sizeable lift internally. See cl/469597645 where I bump to use trailing commas, and the README.google.md file in particular. |
Gotcha, thanks for the context. I'll read through that PR. And I don't have convenient access to google3 for a bit but will hold off on landing this change as-is. The current library adds many more trailing commas than you'd do by hand. I think it would be nice to land on an output format that was at least similar to hand written code. Is there a tracking issue for the dartfmt perf issues w/ large parameter lists? |
Co-authored-by: Nate Bosch <[email protected]>
I super duper agree. And this change definitely makes code more readable. When I first landed "always trailing commas" in this repo, we were in a tight spot internally, so I just implemented a best effort. It would be cool to apply some heuristics (maybe even take into account expression depth, or number-of-characters, ...), but I hadn't put in that effort. I think such a change should be guarded by a performance check. I don't have one offhand though.
I don't see an issue, but I'll cc @munificent. I think the general idea is: in the presence of a trailing comma, the formatter has one less decision to make. But that one decision might have multiple choices, for parameters or arguments, which have a multitude of wrapping possibilities. |
This one works: dart-lang/dart_style#456
It's more complex than that but I don't want to get into it here. But, basically, a trailing comma lets the formatter format each argument entirely independently from the others. Without that, it formats all of the arguments (and all of the splits within those arguments) as one monolithic constraint solve. |
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.
This LGTM.
Before we land it we should run a TAP global and figure out how much breaks. If any tests in third_party break we'll need to make sure we're careful about landing this.
Co-authored-by: Nate Bosch <[email protected]>
For sure; I started an import CL a little bit ago but got a bit hung up on the number of places to update (they all seem to have different ways to test / update their golden masters). |
Hopefully all of the commands are written down in |
I'm going to close this PR for now because - even with updated logic for when to use trailing commas - I still see very slow dart format times for large, generated source files. |
The two whitespace changes aren't critical but they do make the (post-dartfmt) code I'm generating look better. That's dialing back the use of trailing commas for situations where you likely don't need them, and adding blank lines between different types of field groups (static fields and then instance fields).