Skip to content
This repository has been archived by the owner on Nov 1, 2024. It is now read-only.

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

Closed
wants to merge 8 commits into from

Conversation

devoncarew
Copy link
Contributor

  • reduce the use of trailing commas
  • add blank lines between groups of fields
  • rev the minor version of the package (I realized I added new API in the previous PRs)
  • rev to a non-dev version in prep for publishing

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

CHANGELOG.md Outdated Show resolved Hide resolved
@srawlins
Copy link
Collaborator

srawlins commented Dec 20, 2022

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.

@devoncarew
Copy link
Contributor Author

devoncarew commented Dec 20, 2022

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?

Co-authored-by: Nate Bosch <[email protected]>
@srawlins
Copy link
Collaborator

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
Copy link
Contributor

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

This one works: dart-lang/dart_style#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 devoncarew marked this pull request as ready for review February 14, 2023 01:53
Copy link
Contributor

@natebosch natebosch left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Nate Bosch <[email protected]>
@devoncarew
Copy link
Contributor Author

devoncarew commented Feb 20, 2023

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
Copy link
Collaborator

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

@devoncarew devoncarew marked this pull request as draft February 28, 2023 20:52
@devoncarew
Copy link
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants