Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tall style implementation #1403

Open
2 of 19 tasks
munificent opened this issue Feb 21, 2024 · 0 comments
Open
2 of 19 tasks

Tall style implementation #1403

munificent opened this issue Feb 21, 2024 · 0 comments
Labels
meta tall Issues related to the new "tall" style (#1253)

Comments

@munificent
Copy link
Member

munificent commented Feb 21, 2024

This meta issue tracks work remaining to be done to ship the new "tall style" proposal (#1253).

Implement tall style:

  • Implement all language syntax using the new internal representation and formatter. There should be no more throw UnimplementedError in the codebase.

  • (Make the formatter aware of the language version of what it's formatting #1402) Add support for detecting the language version of each file being formatted.

  • Address remaining TODO(tall) and TODO(perf) comments in code. For many of these, we may ultimately decide to do nothing, keep the current behavior, and simply remove the TODOs. But once we have migrated the regression tests and run the new formatter on a large corpus, we should be in a better position to make those decisions.

  • Migrate the regression tests. Since we'll support both the short and tall styles for a while, we'll want to fork them and keep the old ones. There is a test/regression_tall/ directory already, so we can just fork and migrate the test/regression/ subdirectories into there and reformat them using the new tall style. We'll probably want to do it a subdirectory at a time (since it's so much code). The important part is to manually review the formatted results and see if the new style is producing the output we want.

  • Run the new formatter on a large corpus of code. Look at the diffs and evaluate the results. Does it look like we expect? Does it produce weird output on unusual code? Are there style rules or heuristics we want to tweak? Does the performance degrade on some kinds of code?

  • Close issues that are now fixed or invalid. There are likely a bunch of long-standing style issues that the new style either directly fixes or where the issue is now irrelevant because the new style takes an entirely different approach. We can close those out.

  • Get feedback. We can do much of the evaluation ourselves, and we know that most users are generally in favor of the new style. But to minimize churn after it ships, it would help to send out a request for anyone who cares to try out the new style on their code and file issues for output they don't like.

Make migration easier:

When the new style ships, users will start mass migrating code. That can change the indentation of lines, which may in turn cause long strings and comments to go past the page width. Before this feature ships, we may want to consider:

Ship support for tall style:

  • Remove support for dart format --fix. The tall style doesn't support it and now that dart fix supports all of the same changes and can be run in a faster mode with no static analysis, we have no intention of re-implementing it. Instead, we declare a breaking change and remove support for it for both short and tall styles. This means bumping dart_style to 3.0.0.

  • Remove the old dartformat and dartfmt CLI commands and the old CLI. A few years back when we changed the way to run the formatter from dartfmt (its own top-level shell command) to dart format, we also designed a new better set of command line options. That's what you get when you run dart format. But if you globally activate the dart_style package, that still exposes two executables that (for compatibility) continue to use the old CLI.

    Since we are making a 3.0.0 breaking change to dart_style anyway, and (as far as I know), those commands are rarely used, now is a good time to remove them.

  • Ship the experiment. Instead of relying on an experiment flag to determine which style you get, use the language version of the file being formatted. If it's above 3.x (where x is whatever Dart SDK version the new formatter will ship in), you get the tall style. Otherwise, you get the short style.

  • Roll the new formatter into the Dart SDK.

Remove support for short style:

For some number of Dart SDK releases, dart format will support both short and tall styles (and use language version and project configuration to pick which one a user gets where). But we don't indent to support short style on older code in perpetuity. Eventually, we will remove support for short style entirely and even older language version files will be formatted using the tall style.

  • Decide what version of the Dart SDK will no longer support the short style. Coordinate with leads and PMs to decide when we want to remove support.

  • Announce the intent to stop supporting the older style. Send a notification on all the various channels letting users know that the upcoming Dart SDK release will change the formatting of their older language versioned files. Encourage users to pre-emptively configure their project to use the new style on all of their files to minimize disruption.

  • Remove support for short style. Delete the old AST visitor, Chunk internal representation, line splitter, and short style tests. Rejoice at the simpler, cleaner formatter codebase.

  • Roll it into the Dart SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta tall Issues related to the new "tall" style (#1253)
Projects
None yet
Development

No branches or pull requests

1 participant