[tool] Add a way to opt a file out of formatting #4292
Conversation
Hixie
changed the title
|
I made this apply to all files rather than just Dart, mostly because it was easier to plug into the code where |
Hixie
force-pushed
the
format
branch
2 times, most recently
from
b105647 to
596b516
Compare
| return files | ||
| .where((File file) { |
There was a problem hiding this comment.
I made this apply to all files rather than just Dart, mostly because it was easier to plug into the code where
Fileobjects were still around.
I would much rather this apply only to Dart, as this is strictly worse than clang-format off for clang-format-formatted files because the latter will prevent people with editors that are configured to auto-format from accidentally formatting the file when editing it. (I really, really wish dartfmt would support this for the same reason.)
What's the issue with just adding a file.basename.endsWith('.dart') check inside this lambda?
There was a problem hiding this comment.
it seemed weird to have this code know about file extensions too, but i'm happy to add it if you like
script/tool/CHANGELOG.md
Outdated
| @@ -10,6 +10,8 @@ | |||
| `--no-push-flags`. Releases now always tag and push. | |||
| - **Breaking change**: `publish`'s `--package` flag has been replaced with the | |||
| `--packages` flag used by most other packages. | |||
| - Formatting now skips files that contain a line that exactly | |||
| matches the string `// This file is hand-formatted`. | |||
There was a problem hiding this comment.
The period needs to be inside the `s; it's part of the string being searched for.
| // consider it included. It should get filtered out later when we | ||
| // deal with file extensions. (We want the formatters to see the | ||
| // files that we can't read, in case they have opinions about such | ||
| // things as character encodings, etc.) |
There was a problem hiding this comment.
Please don't use "we" in comments. go/avoidwe has the full explanation; the short version is that even within just this last sentence "we" is actually two completely different concepts, which makes it difficult to read.
There was a problem hiding this comment.
please update the style guide to mention this
There was a problem hiding this comment.
since i added the check for the extension, i was able to remove all this error handling logic and thus the comment is gone too
There was a problem hiding this comment.
Do you still want me to add this to the style guide? I'm happy to do so, but it's also a position that not everyone agrees with.
There was a problem hiding this comment.
I don't feel strongly about it (but then I get a guilty pleasure in writing sentences that use the same pronoun to mean different people), but if there's an argument to be made that it makes things clearer, we should document it somewhere people can see it.
Hixie
force-pushed
the
format
branch
3 times, most recently
from
0e63075 to
1a5a4a2
Compare
|
PTAL |
script/tool/CHANGELOG.md
Outdated
| @@ -12,6 +12,8 @@ | |||
| `--packages` flag used by most other packages. | |||
| - **Breaking change** Passing both `--run-on-changed-packages` and `--packages` | |||
| is now an error; previously it the former would be ignored. | |||
| - Formatting now skips files that contain a line that exactly | |||
| matches the string `// This file is hand-formatted.`. | |||
There was a problem hiding this comment.
This got merged into the wrong section, it needs to be in a new one since 0.6.0 is published.
You should actually do a version bump in this PR since in order to use it in flutter/packages it'll need to be published (and then the flutter/packages pin rolled to pick it up)
There was a problem hiding this comment.
Oh, and it should be changed to "skips Dart files".
| // was added because Hixie hurts when dealing with what dartfmt does to | ||
| // artisanally-formatted Dart, while Stuart gets really frustrated when | ||
| // dealing with PRs from newer contributors who don't know how to make Dart | ||
| // readable. After much discussion, it was decided that files in the plugins |
There was a problem hiding this comment.
🤣 Well, I did ask.
(I was just thinking something like "Allow package maintainers to opt out of Dart autoformatting in order to use Flutter's manual formatting style instead.", but this certainly works.)
Hixie
added
the
waiting for tree to go green
Per discussion on Discord, this adds a way to opt-out of the formatting.
Pre-launch Checklist
dart format.)[shared_preferences]///).