Add ability to exclude files

[kentcb]

I'm using dartfmt via flutter format, which I run it like this:

flutter format .

This formats all .dart files recursively, which is almost exactly what I want. Unfortunately, it also formats .g.dart files, so my generated files bounce between their generated state and a formatted version thereof.

It would be very useful if dartfmt could exclude files matching certain patterns. Something like:

dartfmt -e *.g.dart -e *.foo.dart .

(surfacing this via flutter format would of course be a separate issue)

[munificent]

I don't know if it if it's worthwhile to add too much complexity directly in dartfmt for determining what files it runs on. I don't want to have to reinvent and maintain a subset find. It's simpler to use the tools the OS gives us. I think you can accomplish what you want with:

$ find . -name *.dart -not -name *.g.dart -exec flutter format {} \;

[kentcb]

The problem with this approach (which I should have mentioned in my issue) is that I have found it to be much slower. That said, I'm running on Windows and using Grinder. Let me play around with it a little more and report back here.

[kentcb]

Here are some results running dartfmt in various manners against my code base (282 files):

Approach 1: run flutter format .

flutter format .

This takes less than 3 seconds.

Approach 2: run flutter format once per target file

  final files = FileSet.fromDir(
    _rootPath.asDirectory,
    pattern: '*.dart',
    recurse: true,
  );

  for (final file in files.files) {
    await _runFlutter(
      [
        'format',
        file.path,
      ],
    );
  }

This takes nearly 6.5 minutes!

Approach 3: pass all file paths to flutter format

  final fileSet = FileSet.fromDir(
    _rootPath.asDirectory,
    pattern: '*.dart',
    recurse: true,
  );
  final files = fileSet.files.fold(StringBuffer(), (StringBuffer acc, next) {
    acc..write(next)..write(' ');
    return acc;
  });

  await _runFlutter(
    [
      'format',
      '--line-length',
      '120',
      files.toString(),
    ],
  );

This fails with:

The command is too long.

Approach 4: batch files and pass them into flutter format

  final fileSet = FileSet.fromDir(
    _rootPath.asDirectory,
    pattern: '*.dart',
    recurse: true,
  );
  final files = fileSet.files;
  const maxLength = 3072;
  final filesInBatch = <String>[];
  var currentLength = 0;

  for (var i = 0; i < files.length; ++i) {
    final filePath = files[i].absolute.path;

    if (currentLength == 0 || (currentLength + filePath.length + 1) <= maxLength) {
      filesInBatch.add(filePath);
      currentLength += filePath.length + 1;
    } else {
      await _runFlutter(
        [
          'format',
          '--line-length',
          '120',
          ...filesInBatch,
        ],
      );

      filesInBatch.clear();
      currentLength = 0;
    }
  }

  if (filesInBatch.isNotEmpty) {
    await _runFlutter(
      [
        'format',
        '--line-length',
        '120',
        ...filesInBatch,
      ],
    );
  }

This executes in just over 14 seconds, and the code is getting rather complicated for such a simple requirement.

Is this still not something that could be considered for improvement? How can I do this in a cross-platform manner (one of Dart's advantages) without making such large sacrifices in terms of performance and simplicity?

[munificent]

This sounds like a performance issue with flutter format to me. On my Mac laptop, using Flutter v1.10.15-pre.202 to format a tiny file:

$ time flutter format temp.dart
Unchanged bin/temp.dart

real  0m0.957s
user  0m1.063s
sys 0m0.358s

$ time dartfmt -w temp.dart
Unchanged bin/temp.dart

real  0m0.119s
user  0m0.103s
sys 0m0.056s

And formatting the entire Flutter repo:

$ time flutter format .
...
real  0m9.746s
user  0m12.987s
sys 0m1.239s

$ time dartfmt -w .
...
real  0m8.287s
user  0m10.011s
sys 0m0.702s

I don't see why flutter format should add a full second of startup time over dartfmt.

For example, if I run:

$ time find . -name *w*.dart -not -name *_test.dart -exec dartfmt -w {} \;

It matches 112 files and runs in:

real  0m10.525s
user  0m10.389s
sys 0m4.437s

That's not blindingly fast, but it's not intolerable. But when I change it to:

$ time find . -name *w*.dart -not -name *_test.dart -exec flutter format {} \;

Then it takes:

real  1m44.269s
user  2m2.695s
sys 0m37.934s

Which is... really bad. Perhaps it's worth filing an issue on Flutter around the startup performance of flutter format? Their wrapper is outside of my scope and seems to be where most of the overhead is coming from.

How can I do this in a cross-platform manner (one of Dart's advantages) without making such large sacrifices in terms of performance and simplicity?

Maybe I'm asking something dumb but... why not just format the generated files as soon as you generate them? Then you don't have to exclude them.

[kentcb]

why not just format the generated files as soon as you generate them?

This would be awesome, but I'm not sure of any way of doing this. I'm using this command to generate code as I work on my code base:

flutter packages pub run build_runner watch

To my knowledge, there are no options on build_runner that allow you to format the code as it is generated.

[natebosch]

To my knowledge, there are no options on build_runner that allow you to format the code as it is generated.

Not in general. Many builders which emit .dart files choose to formate them within the builder though. What builder are you using? You might follow up with the author and ask that it uses dartfmt internally.

[kentcb]

As per this issue, the problem is that my code base uses a 120 column width rather than the standard, claustrophobic, 80 characters. So built_value (in this case) is formatting because it uses source_gen, but source_gen doesn't provide any means of passing through the column width.

I still feel like this issue is best solved by enhancing dartfmt. However, I understand the reticence to add complexity there - I guess I just kind of assumed that file globbing logic already existed and that it would be a simple matter of leveraging it from dartfmt.

Meanwhile, I don't see any easy path forward for me right now (other than switching to 80 char column width, which I hate). It's worth bearing in mind that I run flutter format as part of my CI to ensure that formatting isn't broken, so a slow-running option would be even less appealing than it already is.

[rockerhieu]

This sounds like a performance issue with flutter format to me. On my Mac laptop, using Flutter v1.10.15-pre.202 to format a tiny file:

$ time flutter format temp.dart
Unchanged bin/temp.dart

real  0m0.957s
user  0m1.063s
sys 0m0.358s

$ time dartfmt -w temp.dart
Unchanged bin/temp.dart

real  0m0.119s
user  0m0.103s
sys 0m0.056s

And formatting the entire Flutter repo:

$ time flutter format .
...
real  0m9.746s
user  0m12.987s
sys 0m1.239s

$ time dartfmt -w .
...
real  0m8.287s
user  0m10.011s
sys 0m0.702s

I don't see why flutter format should add a full second of startup time over dartfmt.

For example, if I run:

$ time find . -name *w*.dart -not -name *_test.dart -exec dartfmt -w {} \;

It matches 112 files and runs in:

real  0m10.525s
user  0m10.389s
sys 0m4.437s

That's not blindingly fast, but it's not intolerable. But when I change it to:

$ time find . -name *w*.dart -not -name *_test.dart -exec flutter format {} \;

Then it takes:

real  1m44.269s
user  2m2.695s
sys 0m37.934s

Which is... really bad. Perhaps it's worth filing an issue on Flutter around the startup performance of flutter format? Their wrapper is outside of my scope and seems to be where most of the overhead is coming from.

How can I do this in a cross-platform manner (one of Dart's advantages) without making such large sacrifices in terms of performance and simplicity?

Maybe I'm asking something dumb but... why not just format the generated files as soon as you generate them? Then you don't have to exclude them.

I'm having performance issue with both dartfmt, flutter format and dart format:

$ time find . -name "*.dart" ! -path "*/generated/*" ! -name "*.g.dart" -exec dartfmt --fix --dry-run --set-exit-if-changed {} \;
find . -name "*.dart" ! -path "*/generated/*" ! -name "*.g.dart" -exec dartfm  19.79s user 11.38s system 143% cpu 21.770 total

$ time find . -name "*.dart" ! -path "*/generated/*" ! -name "*.g.dart" -exec dart format --fix --show none --summary none --output none --set-exit-if-changed {} \;
find . -name "*.dart" ! -path "*/generated/*" ! -name "*.g.dart" -exec dart    88.94s user 32.95s system 88% cpu 2:18.50 total

$ time find . -name "*.dart" ! -path "*/generated/*" ! -name "*.g.dart" -exec flutter format --dry-run {} \;
find . -name "*.dart" ! -path "*/generated/*" ! -name "*.g.dart" -exec flutte  168.47s user 74.83s system 110% cpu 3:39.73 total

It is really really fast when I run the command directly:

$ time dartfmt --fix --dry-run --set-exit-if-changed lib test test_driver                                                                                                               
dartfmt --fix --dry-run --set-exit-if-changed lib test test_driver  1.10s user 0.28s system 158% cpu 0.870 total

$ time dart format --fix --show none --summary none --output none --set-exit-if-changed lib test test_driver
dart format --fix --show none --summary none --output none  lib test   1.91s user 0.47s system 123% cpu 1.923 total

$  time flutter format --dry-run lib test test_driver
flutter format --dry-run lib test test_driver  2.63s user 0.79s system 132% cpu 2.588 total

Some more info about the project:

• The project has about 100 files.
• dartfmt: 1.3.7
• dart: 2.10
• flutter: 1.22.0

@munificent dartfmt + find takes 20s (while it should be less than a second) which is too slow. It would be great if you consider to include a flag for excluding files.

[rockerhieu]

@kentcb I found a way around this:

• Find all unformatted files at once (without changing anything --output none):

dart format --fix --show changed --summary none --output none lib test test_driver

It should have an output like this:

Changed lib/a.dart
Changed lib/generated/b.dart
Changed lib/data/c.g.dart.dart

• Go through the output and let the build fail if any of the file is not in the whitelist:

mapfile -t unformatted_files < <(dart format --fix --show changed --summary none --output none lib test test_driver)
for file in "${unformatted_files[@]}";  do
  file=${file#"Changed "}
  # Warn about the unformatted file if it's not in the whitelist
  if [[ \
        ! "$file" = *".g.dart" \
     && ! "$file" = *"/generated/"* \
     ]]; then
     echo
     echo "\"$file\" is not properly formatted"
     echo "Please run the below command to format it:"
     echo
     echo "    dart format --fix $file"
     echo
     exit 1
  fi
done

@munificent this is rather hacky and is not stable (depending on how stable the output of --show changed is). Would be nice if the performance issue is fixed, or have a built-in flag for excluding files.

[brunobowden]

@munificent - the startup time for each individual flutter format process destroys the performance (nearly 40X slower for me). My workaround was to convert the find output from multiple lines to a single line which only takes 1.6X the time. I'm leading the team developing the COVID-19 app for WHO (GitHub) and this issue just came up. I'd much rather supply exclude flags for pattern matches on directories and filenames, so I would support keeping this issue open.

@kentcb - I'd suggest documenting a few of these approaches in the issue description so that others looking at this bug can get the suggestions. I appreciate my method only works for repos of modest size.

# 1.6X with exclude: multi line => single line
$ time find . -name "*.dart" ! -path '*/generated/*' ! -path './proto/*' | tr '\n' ' ' | xargs flutter format
real	0m4.876s

# normal case
$ time flutter format .
real	0m2.904s

# 40X time with multi line pipe
$ time find . -name "*.dart" ! -path '*/generated/*' ! -path './proto/*' -exec flutter format {} \;
real	1m51.976s

[munificent]

This is maybe a silly question, but is there a reason you don't just go ahead and format the generated files? Doing so is harmless, probably makes them easier to read, and lets you just run dartfmt on your whole source tree. This is generally what we do for all of the Google-maintained Dart projects.

[brunobowden]

@munificent - this is a good point and on second thoughts I agree with you. I didn't realize this before but the WHO App codebase already does what you suggested of formatting the generated files.

@kentcb - I still think it's worth summarizing some of these thoughts in the description, so people can learn good strategies for this.

[enyo]

@munificent I'm looking for a similar solution because I'm using grpc with protobuf and the generated files aren't formatted. So I need to run the formatter every time the protoc build runs (which happens automatically when a proto file is changed). That makes it easy to forget and annoying, but I guess it's more an issue with the protobuf library than the formatter.

[mbaz2]

I am running into similar issue with my Github Action. Trying to run flutter format lib/, but this includes generated translation files. I want the Action to cease if there is a format so I use flutter format lib/ --set-exit-if-changed, but this exits because of the generated files. @munificent my understanding would be best practice is to not check generated files into the repo. I can explicitly list every sub directory, but would be far easier to just exclude the generated file vs. having to list every directory that needs to be formatted. Thoughts?

[enyo]

@mbaz2 you could run dart format in the newly generated directories in your GitHub action after generating the files.

[awhitford]

my understanding would be best practice is to not check generated files into the repo.

Often that is true. However, l10n/messages* are generated and committed.

I too am interested in an exclude directory feature for flutter format so that I can exclude the l10n generated code.

[munificent]

Often that is true. However, l10n/messages* are generated and committed.

The easy fix here is to generate them, format them, then commit that.

[awhitford]

The easy fix here is to generate them, format them, then commit that.

I'm trying to convince my colleague of that. 🤞

[DeividWillyan]

@munificent - the startup time for each individual flutter format process destroys the performance (nearly 40X slower for me). My workaround was to convert the find output from multiple lines to a single line which only takes 1.6X the time. I'm leading the team developing the COVID-19 app for WHO (GitHub) and this issue just came up. I'd much rather supply exclude flags for pattern matches on directories and filenames, so I would support keeping this issue open.

@kentcb - I'd suggest documenting a few of these approaches in the issue description so that others looking at this bug can get the suggestions. I appreciate my method only works for repos of modest size.

# 1.6X with exclude: multi line => single line
$ time find . -name "*.dart" ! -path '*/generated/*' ! -path './proto/*' | tr '\n' ' ' | xargs flutter format
real	0m4.876s

# normal case
$ time flutter format .
real	0m2.904s

# 40X time with multi line pipe
$ time find . -name "*.dart" ! -path '*/generated/*' ! -path './proto/*' -exec flutter format {} \;
real	1m51.976s

Thanks for your answer, I adapted to exclude the generated * .g.dart classes and adapted with --set-exit-if-changed to run in my pipeline.

find . -name "*.dart" ! -name "*.g.dart" ! -path '*/generated/*' ! -path './proto/*' | tr '\n' ' ' | xargs flutter format --set-exit-if-changed

[pr0gramista]

I still find this option worth implementing.

The codebase for my medium sized app takes 1m 22s to format, way too long for snappy feeling. This is because we are formatting protobuf generated classes.

time flutter format .  
85,75s user 1,81s system 106% cpu 1:22,59 total

It is over 15x faster to remove these generated classes, format the code and regenerate them back.

time flutter format .  
4,06s user 1,00s system 132% cpu 3,821 total
time protoc -I protos/ protos/*.proto --dart_out=../lib/models/generated  
0,60s user 0,14s system 121% cpu 0,607 total

And we are talking about relatively fast CPU not some slow CI machine.

I realize that it is not common for someone to format entire codebase every time, but I see few cases where that could be useful like pre-commit checks, coding without IDE, formatting multiple projects.

[munificent]

The codebase for my medium sized app takes 1m 22s to format, way too long for snappy feeling. This is because we are formatting protobuf generated classes.

Can you provide some details on which generated classes are the slowest? It should never take dartfmt that long to format even a very large repo, but there are a couple of edge cases (issues) where the perf degrades significantly. I'd rather spend time fixing those performance bugs than spend time adding features that let users work around those bugs.

[pr0gramista]

Can't share my file, but if I generate class from Google APIs Protobufs it has similar time/file size.

https://gist.github.com/pr0gramista/8dfa8d653f3cd5dd075eeb5c36dbe0ca

time flutter format cloudscheduler.pb.dart
3,25s user 0,32s system 115% cpu 3,105 total

https://gist.github.com/pr0gramista/5a9615f1a4bfb40a4b9b3301bf8cc78f

time flutter format appengine.pb.dart
16,68s user 0,50s system 106% cpu 16,133 total

One of my generated file with few classes in it has ~2500 LOC and takes 17s, a bit longer than expected - maybe because of how nested our structures are. I'll post it if I manage to obscure the contents.

[ArtemZip]

Often that is true. However, l10n/messages* are generated and committed.

The easy fix here is to generate them, format them, then commit that.

I don't want to commit generated code as I can change anything in my code and half of repository will be automatically changed due to code generation

[munificent]

In the comment I was replying to, @awhitford said they already were committing the generated files, which is why I suggested formatting before committing.

In your case, you can set up your build system to format after generating and then simply not commit the results.

[lukepighetti]

Recently ran into this issue. We are using Mason bricks in our project which uses Dart files that do not have valid dart syntax due to the use of Mustache templating engine. We are able to exclude these from analyzer with this:

analysis_options.yaml

analyzer:
  exclude:
    - bricks/**/__brick__

But have no such tools to denylist that same folder glob for dartfmt

This makes our CI tools fail because dartfmt tries to format *.dart files that don't have valid dart syntax.

cc @felangel

[felangel]

Recently ran into this issue. We are using Mason bricks in our project which uses Dart files that do not have valid dart syntax due to the use of Mustache templating engine. We are able to exclude these from analyzer with this:

analysis_options.yaml

analyzer:
  exclude:
    - bricks/**/__brick__

But have no such tools to denylist that same folder glob for dartfmt

This makes our CI tools fail because dartfmt tries to format *.dart files that don't have valid dart syntax.

cc @felangel

@lukepighetti the way I've worked around this in the past is to isolate the templates in a separate top-level directory (like bricks) and only run the dart formatter from within the context of the relevant dart packages. Can you provide a bit more context regarding your overall project/directory structure?

[lukepighetti]

The reason I'm excluding just that subpath is because we wanted analysis on the dart post/pre gen hooks, although we're getting off topic now

[srawlins]

Wouldn't dart format lib/ test/ etc work?

[lukepighetti]

you mean dart format lib test bin tool packages in multiple package roots that may not have all of those items? 😅

[fabriziocacicia]

Recently ran into this issue. We are using Mason bricks in our project which uses Dart files that do not have valid dart syntax due to the use of Mustache templating engine. We are able to exclude these from analyzer with this:

analysis_options.yaml

analyzer:
  exclude:
    - bricks/**/__brick__

But have no such tools to denylist that same folder glob for dartfmt

This makes our CI tools fail because dartfmt tries to format *.dart files that don't have valid dart syntax.

Ran into the same issue just today. I have a 'dart analyze and format' job running as Action which fails due to dart format checking the bricks folder. In my case, I use a base template for all of my Dart projects which includes the dart CI workflow.

As mentioned by @srawlins I could easily edit the workflow in the base template by limiting dart format to the three most common directories, i.e. lib, test, and example. Which is kinda okay but not ideal, because in some repos I have other directories I want to check, such as layers or features, that are at the same level as lib and I should make the Dart CI workflow specific for each single one of them by adding the additional directories. Instead, excluding some common directories and leaving all the others to be checked by dart format would be more convenient.