Support excluding some files from formatting

[annagrin]

Describe the bug Setting dart.doNotFormat does not seem to work.

To Reproduce Steps to reproduce the behavior:

1. Add the following lines to settings.json

"dart.doNotFormat": [
        "**/fixtures/**",
        "**/flutter/flutter/**"
    ],
    "editor.defaultFormatter": "Dart-Code.dart-code",
    "editor.formatOnSave": true,

Expected behavior Code does not get formatted on save in files /User/<name>/source/webdev/fixtures

Please complete the following information:

• Operating System and version: MacOS 13.2.1, Apple M1 Max
• VS Code version: Version: 1.76.2 (Universal)
• Dart extension version: v3.62.0
• Dart/Flutter SDK version: Flutter (Channel unknown, 3.9.0-20.0.pre.33, on macOS 13.2.1 22D68 darwin-arm64, locale en-US)

[DanTup]

This is sort-of a dupe of #3586 which I closed as unplanned. However, it's come up a few times recently (including from @jakemac53) and I think may be worth reconsidering.

Before LSP, when all formatting was done by the extension directly, I implemented this setting as a way to avoid certain files being formatted (such as generated files).

When we moved to LSP, I never implemented this there because it wasn't clear there was much demand, and it felt slightly awkward adding support for it to the server in a way that only worked for LSP/VS Code (it would be nicer if it was in some editor-agnostic config).

Since then, I learned about LSP middleware. In theory, we could re-implement this there (essentially dropping any format requests on the way to the server if the current file matches these globs). However, I recently noticed @bwilkerson committed some support for disabling formatting for dart fix here and wonder whether it would make sense to consolidate these things.

@bwilkerson I don't know anything about the dart fix support for formatting, but how would you feel about expanding this to a) also apply to general server format requests and b) support globs?

[bwilkerson]

I didn't even remember that we'd done that. I'd like us to revisit that experiment and think about the best way to support formatting (or not) in all of the places where we need it so that we don't end up supporting multiple mechanisms, but I won't have cycles for that for a little while. You're welcome to run with it if you want.

[jakemac53]

We should also try and get @munificent on board for support in dart format as well, so that it is reasonable to run CI jobs that enforce formatting on the formatted portions of a repo only. A single point of configuration here would be a big win.

[annagrin]

Thought I'd mention my use case - I frequently need to turn off all formatting in VSCode when I switch from dart sdk repo to flutter one. Also, some of our tests require special formatting since they are testing the source code locations as seen by the debugger.

Would be great to have this feature working!

[DanTup]

@annagrin FWIW, you can disable formatting for an entire project/workspace by running the Preferences: Open Workspace Settings command from the palette (F1) and setting the "dart.enableSdkFormatter": false, setting.

This will be written into a .vscode/settings.json file in the folder, or if you're using a .code-workspace file, it'll go in there.

This will completely disable th formatter, but another option is just to stop it running automatically:

"[dart]": {
    "editor.formatOnSave": false,
    "editor.formatOnType": false,
    "editor.formatOnPaste": false
},

If you do that, you'll still be able to use it manually (such as using the "Format Selection" command), but it won't run on save/paste/typing.

(it doesn't cover all the use cases of the previous dart.doNotFormat setting, but you might find it helpful in the meantime)

[jakemac53]

Yeah, the workspace settings are what I have typically done, but it's easy to forget to set up and I don't personally use saved workspaces much. So I often run into this issue still :(.

Fwiw I think another argument for a glob based feature here is to support not formatting generated files. Modifying those files in build_runner is not supported, and the behavior is undefined.

[DanTup]

Thoughts on having a formatter: section in analysis_options that has an exclude that works similar to the analyzer's exclude?

analyzer: # existing
  exclude: 
    - '**/*.g.dart'
format(ter?): # new
  exclude: 
    - '**/*.g.dart'

We'd use the existing Glob class the analyzer uses to compare these. We could have some shorthand for exclude everything without a glob:

format: false

or

format:
  enabled: false

LSP server would just return empty edits for any file that matches the excludes (and if analyzer supports includes that can override excludes, we should do the same). And ideally, dart format would do the same.

[munificent]

I'm not morally opposed to support file exclusions in the formatter. But it is a somewhat hard engineering change because it means the formatter needs to start looking around in the file system to try to find a project-level config file. That kind of behavior can be brittle (what if an IDE integration just invokes the formatter with a string of code as text without a file path?) and can also fail in weird ways (what if a parent directory is unreadable?).

It may still be worth doing, but it is really nice that the formatter today can work purely on strings and individual files. It keeps it much simpler and more robust.

[jakemac53]

But it is a somewhat hard engineering change because it means the formatter needs to start looking around in the file system to try to find a project-level config file.

There are definitely lots of complications with this - you also have to consider that these files can be nested under each other and define the behavior. The easiest thing is a pure override and not attempting to do any merging, but you will then get requests to support importing other config files to reduce duplication (and then have to define the merging behavior of that).

It also means every time you navigate into a directory you need to check for a config file in that directory as well and possibly override the current config. This gets especially weird if you consider symlinks.

It may still be worth doing, but it is really nice that the formatter today can work purely on strings and individual files. It keeps it much simpler and more robust.

Fwiw I do think it is totally reasonable to just not attempt to load configuration when given a simple string. It does get weird if you are given a string and path though - you might just want this api to accept a configuration object.

[DanTup]

The easiest thing is a pure override and not attempting to do any merging

From a quick test, this appears to be what the server does for excludes in analysis_options.yaml (I think because each analysis_options.yaml creates a new context and doesn't look at parents) - so if we put this config in this file, this behaviour would be consistent.

Even if this isn't a good fit for dart format, I think there's a lot of value in having this in server for IDEs. People often have format-on-save enabled there and this can be disruptive, whereas running dart format is something done much more consciously. We had it in Dart-Code in the past, and it has come up a few times since we lost it in the LSP move.

@bwilkerson do you have any opinions on this (for example doing something like the above)?

[bwilkerson]

... I think because each analysis_options.yaml creates a new context and doesn't look at parents ...

That's correct, though we do have the complication that one options file can 'include' another, in which case we do perform a merge operation with the included file.

As for what we should do, I don't have a strong opinion because I'm not sure I understand the use cases here well enough. There are a few places where we clearly shouldn't run the formatter: generated code and the flutter packages. The first should probably be baked in, the second we have a work-around for, though there might be better ways to handle it. The question is, are there other use cases for wanting to exclude some files from being formatted?

[DanTup]

The first should probably be baked in

Do we have a common enough convention for generated code to be able to bake this in?

are there other use cases for wanting to exclude some files from being formatted?

I don't know if significant/common, but results of a quick search:

• in @annagrin's example above there is "**/fixtures/**", but I don't know the contents of that folder - is it purely generated code (that we could detect as such), @annagrin?
• https://github.com/Dart-Code/Dart-Code/issues/3703 asks to disable for a single file - I think because of the wrapping (I don't think increasing line-length for a single file is possible)
• Dart-Code's tests used this to allow some golden test files for syntax-highlighting to be manually formatted
• I know @jakemac53 was interested in this but I don't know if his use cases are covered by the above?

[bwilkerson]

Do we have a common enough convention for generated code to be able to bake this in?

Server uses the following suffixes to mean that the file was generated:

• '.g.dart'
• '.pb.dart'
• '.pbenum.dart'
• '.pbserver.dart'
• '.pbjson.dart'
• '.template.dart'

That list might not be complete, but at the very least we'd be treating the same files as generated for this purpose as we do for any other purpose.

The test is done by the function isGenerated in file_paths.dart.

[jakemac53]

The list is definitely very incomplete, especially for external flutter users. I would discourage doubling down on an approach like that.

[bwilkerson]

Given that we use the list to control other features, (a) is there a better way to determine that a file is generated, or (b) what are the other common patterns that we should add to the list and how do we prevent the list from getting outdated in the future?

[jakemac53]

I don't have a better alternative suggestion - I have a roughly equally flawed suggestion that any file with multiple extensions is probably generated (ie: foo.bar.dart). But it depends if you care more about eliminating false positives or negatives more which approach would be better.

The tldr; is really just that we don't have a good way to know, so I wouldn't base formatting decisions on it by default.

[DanTup]

FWIW, in both DAP and LSP in the SDK there are generated files that end with _generated.dart. We can easily rename these, but perhaps if we've done it here, there are many others.

I also wonder whether we really want to tie these things together - maybe there are cases where a user would want to format generated code? (this only really applies if dart format used these rules too, but I think whatever the solution should probably allow for that even if it's not planned now).

It's also possible by using these rules, users might be motivated to name files in odd ways specifically to exclude them from formatting. For example in https://github.com/Dart-Code/Dart-Code/issues/3703 the user could rename their non-generated file to screens.g.dart to get the behaviour they'd like.

[munificent]

maybe there are cases where a user would want to format generated code?

Turning this around, why wouldn't a user want to format generated code? Unless the generator is working really hard to output nice code, the odds of the formatter making it worse seem very very slim to me. Generated code was actually one of the main motivations for having an automated formatter at all.

[DanTup]

I don't know (my generated code tends to be formatted), but some possible thoughts:

• the code generation is such that it's not convenient to run the formatter at the end of it
• formatting just wasn't thought about or noticed until a manual edit and save caused a big edit (at which point it might not be the right time to take a detour to add formatting and additional commits)
• the generated code is such that the formatting can be slow (perhaps unlikely, but the LSP code in the analysis_server had to be massaged a bit to improve performance because it had very long expressions that were convenient to generated, but hard for the formatter to wrap)

I'm not sure those are good reasons, but perhaps adds to the argument that being generated and not formatted shouldn't be coupled together.

Btw, something that occurred to me but wasn't noted above, is that we could also control this only for automated (on-save/on-type/on-paste). Eg. allow you to prevent format-on-save for some files, without just refusing to format them entirely (eg. you can still manually format the document or a range). This would probably only make sense in the context of LSP though.. having a general-looking setting that only applies to some specific features might be confusing.

(there's also an argument for VS Code/LSP clients supporting the formatOnSave/formatOnPaste/formatOnType settings for specific globs too, but I think that's a long-standing request)

[jakemac53]

Turning this around, why wouldn't a user want to format generated code?

This can cause problems for build systems which are trying to do incremental builds - generally modifying outputs of such build systems is not supported.

We do support formatting code (via the dart_style library) in the build process itself, and that is the default behavior for source_gen, but you can't then customize the formatting, and also the version of the formatter used might be different from the one shipped with your SDK. So if the IDE formats the code it may alter it, which may invalidate build actions unnecessarily (or cause errors or other undefined behavior).

[TimWhiting]

Sounds like supporting the setting so that the generators format the same as the IDE would fix the incremental build issue here. Also sounds like the includes / excludes option would be highly desired.

[rrousselGit]

I personally would like this because I use a Dart mono-repo, and I want to exclude only some specific folders from formatting.
Disabling dart formatting for the workspace would therefore not work for me.

My use-case is documentation. Snippets for the documentation aren't written in the .md files but rather on separate .dart files, so that Dart snippets can be statically analyzed (to prevent errors as the package updates).
But the source file often has various custom comments in there used to enable the docs to trim useless content.

For example, to render:

AsyncValue<Activity> activity = ref.watch(
  // Some explanation
  activityProvider('recreational'),
);

The documentation has a .dart file corresponding to:

// ignore_for_file: omit_local_variable_types, unused_local_variable, prefer_final_locals

import 'package:flutter/material.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';

class Activity {}
final activityProvider = FutureProvider<Activity>((ref) => throw UnimplementedError());

class Example extends ConsumerWidget {
  const Example({super.key});

  @override
  Widget build(BuildContext context, WidgetRef ref) {
/* SNIPPET START */
AsyncValue<Activity> activity = ref.watch(
  // Some explanation
  activityProvider('recreational'),
);
/* SNIPPET END */

    return Container();
  }
}

But as you can notice, formatting doesn't quite run on this file. If I were to format it, the documentation output would change in an undesired way.

[bsutton]

My use case is that I'm building a dart source obfuscator.

For testing, I have two associated fixtures - the pre-obfuscated code and the post-obfuscated code. The post-obfuscation code is a type of golden image and must not be modified in any way.

Formatting is constantly breaking the post-obfuscated code because of things like comments being removed and the formatter then collapsing empty lines.

So my preference is that this should be made explicit and the analysis.yaml file seems like the logical place.

[woolfred]

To throw in another example: We have some SVG assets and would like to reference them via an Assets class with some static const fields. To make it easier to compare the paths and spot mistakes, it would be nice if they could be aligned. But this is just a single file thing for us.

// Harder to spot something is off
class Assets {
  static const shortImage = 'asets/tutorial/shortImage.svg'
  static const longerDescImage = 'assets/tutorial/longDescImage.svg'
}

// Easier to spot something is off
class Assets {
  static const shortImage      = 'asets/tutorial/shortImage.svg'
  static const longerDescImage = 'assets/tutorial/longDescImage.svg'
}

[HonzaKubita]

I would also like to add to this. I wish we had an option to disable the formatter for specific files with some special comment. I have this issue: I have this ColorFilter matrix and before formatting it looks like this:

import 'package:flutter/material.dart';

final grayscaleFilter = const ColorFilter.matrix(<double>[
  0.2126,0.7152,0.0722,0,0,
  0.2126,0.7152,0.0722,0,0,
  0.2126,0.7152,0.0722,0,0,
  0,0,0,1,0,
]);

and after format it gets transformed into this:

import 'package:flutter/material.dart';

final grayscaleFilter = const ColorFilter.matrix(<double>[
  0.2126,
  0.7152,
  0.0722,
  0,
  0,
  0.2126,
  0.7152,
  0.0722,
  0,
  0,
  0.2126,
  0.7152,
  0.0722,
  0,
  0,
  0,
  0,
  0,
  1,
  0,
]);

And i don't want to disable and re-enable the autoformat setting each time I want to make a change to this file

something like

// dart-disable-formatting
import 'package:flutter/material.dart';

final grayscaleFilter = const ColorFilter.matrix(<double>[
  0.2126,0.7152,0.0722,0,0,
  0.2126,0.7152,0.0722,0,0,
  0.2126,0.7152,0.0722,0,0,
  0,0,0,1,0,
]);

[munificent]

When the new formatting style ships, you will be able to use a pair of comments to disable formatting a region of code.

[act262]

When the new formatting style ships, you will be able to use a pair of comments to disable formatting a region of code.

// dart format off

// dart format on

This is very useful

[srmncnk]

@munificent That sounds great. When is this going to be released? Will it come automatically with new Flutter as well?

[munificent]

@munificent That sounds great. When is this going to be released?

Unless something goes wrong, it will ship in Dart 3.7, and in the Flutter release that corresponds to that.