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

Consider disabling formatting in generated files by default #3812

Open
rrousselGit opened this issue Feb 1, 2025 · 10 comments
Open

Consider disabling formatting in generated files by default #3812

rrousselGit opened this issue Feb 1, 2025 · 10 comments
Assignees

Comments

@rrousselGit
Copy link

Formatting is fairly expensive for code-generation.
Although it's useful to make badly generated code more readable, it has a big cost.

Consider

  • have dartfmt not check generated files by default (by ignoring .g.dart?)
  • have build not format those
  • suggest builder authors to have their generated code be humanly readable.
@davidmorgan
Copy link
Contributor

Thanks! Yes, we've noticed formatting can be a nontrivial cost.

There is a trick here: by far the most important case is if the generated code hits a bad performance case in "short style" dartfmt. You can tweak the generated code to make it much, much faster. Usually this means adding some trailing commas.

Here is that fix on build_value. I'd like to check if common generators have the same problem, but haven't gotten around to it. In any case it stops being a problem with the Dart 3.7 "tall style" which just adds and removes commas as it likes, so it will become much less important.

Once that's taken care of, formatting is a very small part of build_runner cost, so it doesn't need optimizing yet. But when build_runner is fast it might be interesting. Formatting parallelizes well, so I think we can reach a state where we do format everything and it's fast.

@davidmorgan davidmorgan self-assigned this Feb 1, 2025
@jakemac53
Copy link
Contributor

As far as not formatting generated files I believe the formatter supports some way of opting out files (or at least regions of files) now? cc @munificent to confirm if that is a thing...

If we had that then the guidance should just be for builders to opt out their own code using that.

@rrousselGit
Copy link
Author

If we go for this, I think it'd be worth to change the default behaviour and have dart format not check any *.*.dart

The fewer configuration folks have to do, the better.

@davidmorgan
Copy link
Contributor

I think the end state we're aiming for is that everything is formatted and everything is fast.

If generated code doesn't hit dartfmt bugs, it can be fast; if it's not fast enough, it can run in a different process.

And: if code has already been formatted, such as generated output on disk, there's no need to format it again :)

@jakemac53
Copy link
Contributor

If we go for this, I think it'd be worth to change the default behaviour and have dart format not check any *.*.dart

I disagree, I think it would be very weird for the formatter to bake in assumptions like this. I have definitely seen people use multiple file extensions before for non-generated files.

It should just be configured, but by builder authors and not all users, so there isn't a lot of toil involved.

@rrousselGit
Copy link
Author

If formatting should be disabled by default for generated files, then we need a way to tell dartfmt not to fail on those.
If excluding *.*.dart isn't good, then we could have it detect other things. Like:

@generated
library;

Or:

// ignore_for_file: dartfmt

Or something else included in the file to notify that it's generated.

If the formatter doesn't exclude generated files by default, then generated files should be formatted by default.
I see no in-between. The two tools should have a matching default behaviour IMO.

@jakemac53
Copy link
Contributor

If formatting should be disabled by default for generated files, then we need a way to tell dartfmt not to fail on those.

Yes this is exactly what I said above, I believe it already supports opting out a file and/or region. But if not we should try and push on that.

I think it would be ideal actually for all builders to always add this, because even if they do format their output you don't want the command line formatter also trying to format it (technically, they could be different versions).

@rrousselGit
Copy link
Author

We agree then. My point was more about having build/source_gen disable formatting themselves.
I think a convention here is important.

@jakemac53
Copy link
Contributor

The source_gen combined builder should definitely just do this by default, which will cover many use cases, and we can document for the custom use cases that it is best practice to do this for sure.

@munificent
Copy link
Member

Yes this is exactly what I said above, I believe it already supports opting out a file and/or region.

Yes, the syntax looks like:

// dart format off
nothing + here + gets + formatted;
// dart format on

If you want to disable formatting the whole file, you can just put // dart format off at the top and not close it at the end.

Note that this only works with the new short style, so it will only help generated code whose target language version is 3.7 or higher.

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

No branches or pull requests

4 participants