dart-lang / code_builder

A fluent API for generating valid Dart source code
https://pub.dev/packages/code_builder
BSD 3-Clause "New" or "Revised" License
423 stars 66 forks source link

reduce the use of trailing commas; add blank lines between groups of fields #390

Closed devoncarew closed 1 year ago

devoncarew commented 1 year ago

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).

srawlins commented 1 year ago

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 dart format.

Also see much discussion on the PR in which trailing commas were added: https://github.com/dart-lang/code_builder/pull/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.

devoncarew commented 1 year ago

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 dart format.

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?

srawlins commented 1 year ago

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.

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.

Is there a tracking issue for the dartfmt perf issues w/ large parameter lists?

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.

munificent commented 1 year ago

I don't see an issue, but I'll cc @munificent.

This one works: https://github.com/dart-lang/dart_style/issues/456

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.

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.

devoncarew commented 1 year ago

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.

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).

srawlins commented 1 year ago

Hopefully all of the commands are written down in README.google.md. If not, please update.

devoncarew commented 1 year ago

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.