dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.59k forks source link

[Meta] Support project-wide dart format page width configuration #56863

Open munificent opened 1 month ago

munificent commented 1 month ago

After the new tall style issue, the two formatter issues with the most 👍 are both about configuring page width (1, 2). This has been the #1 feature request for many years with hundreds of users asking for it.

So we are adding support for project-wide page width configuration to the formatter. After weighing a number of ways to author this configuration, the best approach seems to be a combination of two options. Each helps address the problems of the other:

Look for a surrounding analysis_options.yaml file and let it configure the page width

When formatting a file on disk using dart format, we walk up the surrounding directories looking for an analysis_options.yaml file. If we find one and it has a section like:

formatter:
  page_width: 123

Then the file is formatted at that width.

This lets users author the width in one location in a parent directory (likely the root directory of a package), and then all files in that directory tree get formatted the same way. Since analysis_options.yaml files also support an include key to import another analysis_options.yaml file, organizations have a way to have a standardized formatting style shared across all of their projects.

The main problem with using analysis_options.yaml (or any other config file) is that code generators and other tools that format code that only exists in memory don't know what config file to use. If the code generator doesn't use the configured width, but the user then runs dart format on the whole directory, the generated code will be changed. Likewise, if CI tries to validate that every file is formatted using the configured width, that will fail because the generated stuff is formatted at a different width.

Allow a comment inside of a source file to indicate the page width for that file

If you put // dart format width=123 at the top of a file, then the file is formatted at that page width. This is simple to implement, and works with dart format, code generators, and any other tool that wants to format code. It works even without having access to file IO or any knowledge of where the formatted code will end up on disk.

But it's very tedious for users to have to remember to put this in every single file they author. If they forget, they get inconsistently formatted code. No user has asked for this. They want project-wide configuration.

Hybrid approach

We implement both of these. If there is both a project-wide page width set in analysis_options.yaml and the file has a page width comment, the latter takes precedence (just like a // @dart=1.2 language version comment takes precedence over the SDK constraint in a pubspec).

Mixing both approaches lets us avoid the problems with each:

This approach does mean that generated files may have a different width than the user prefers. But these files are fairly rarely read, so that's a tolerable compromise to avoid having every library that works with generated code and the formatter become obligated to support searching for and reading analysis_options.yaml (which isn't possible in some contexts like build_runner).

Implementation tasks

The core formatter work is done now (but behind a flag). The work remaining is largely in other packages:

munificent commented 1 month ago

@dart-lang/analyzer-team, @dart-lang/language-team, @dart-lang/core-package-admins, any other important tools/workflows/packages that should handle this?

parlough commented 1 month ago

If you're going to use formatter as the top-level key (which makes sense), perhaps that name should be reserved on pub.dev. It seems analyzer plugins (and other plugin-like tools) often use top-level keys matching their package name for configuration.

kevmoo commented 1 month ago

@parlough 's comment reminds me about a similar discussion we had about namespacing "properties" in pubspec.yaml, too.

munificent commented 1 month ago

Good idea, @parlough. Filed an issue and added it to the list. Thanks!

sigurdm commented 1 month ago

We should also ensure that pana respects that setting.