| Auto-Submit | +1 |
| Commit-Queue | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
# Note: Updates to dart_style have to be coordinated with the infrastructure
# team so that the internal formatter `tools/sdks/dart-sdk/bin/dart format`
# matches the version here. Please follow this process to make updates:
#
# * Create a commit that updates the version here to the desired version and
# adds any appropriate CHANGELOG text.
# * Send that to eng-prod to review. They will update the checked-in SDK
# and land the review.
#
# For more details, see https://github.com/dart-lang/sdk/issues/30164.I think it's time to take the practically non-existing EngProd team out of the loop for dart_style rolls.
Maybe the process could be:
* Bump DEPS
* Build SDK
* Run <script that formats all files we want to reformat using locally built SDK> (this script would need to be created)
-> the presubmit still using the checked-in SDK now treats those files as having pre-existing formatting issues and ignores their formatting.
* Bump the checked-in SDK as soon as the regular dev releases (Tue/Thu) pick up the dart_style change.
-> the presubmit and locally built SDKs are now in sync again.
That would also ensure that all non-formatted files that somehow slipped through would be formatted whenever we roll dart_style.
WDYT?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Also, the test failures appear to be caused by this CL as well, PTAL at those before landing.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
# Note: Updates to dart_style have to be coordinated with the infrastructure
# team so that the internal formatter `tools/sdks/dart-sdk/bin/dart format`
# matches the version here. Please follow this process to make updates:
#
# * Create a commit that updates the version here to the desired version and
# adds any appropriate CHANGELOG text.
# * Send that to eng-prod to review. They will update the checked-in SDK
# and land the review.
#
# For more details, see https://github.com/dart-lang/sdk/issues/30164.I think it's time to take the practically non-existing EngProd team out of the loop for dart_style rolls.
Maybe the process could be:
* Bump DEPS
* Build SDK
* Run <script that formats all files we want to reformat using locally built SDK> (this script would need to be created)
-> the presubmit still using the checked-in SDK now treats those files as having pre-existing formatting issues and ignores their formatting.
* Bump the checked-in SDK as soon as the regular dev releases (Tue/Thu) pick up the dart_style change.
-> the presubmit and locally built SDKs are now in sync again.That would also ensure that all non-formatted files that somehow slipped through would be formatted whenever we roll dart_style.
WDYT?
SGTM. Thanks for pushing back on this. I wrote a process here:
https://github.com/dart-lang/dart_style/wiki/Release-process
How does that look?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
I like the suggestion to reformat the SDK as part of this roll. I gave that a try but unfortunately ran into a regression in the formatter that would break static error tests by putting a blank line above the marker comments.
I have a fix for that out: https://github.com/dart-lang/dart_style/pull/1862
Once that lands, I'll update this CL to point to that commit, and reformat the SDK (and CFE golden files) as part of the roll.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
# Note: Updates to dart_style have to be coordinated with the infrastructure
# team so that the internal formatter `tools/sdks/dart-sdk/bin/dart format`
# matches the version here. Please follow this process to make updates:
#
# * Create a commit that updates the version here to the desired version and
# adds any appropriate CHANGELOG text.
# * Send that to eng-prod to review. They will update the checked-in SDK
# and land the review.
#
# For more details, see https://github.com/dart-lang/sdk/issues/30164.Bob NystromI think it's time to take the practically non-existing EngProd team out of the loop for dart_style rolls.
Maybe the process could be:
* Bump DEPS
* Build SDK
* Run <script that formats all files we want to reformat using locally built SDK> (this script would need to be created)
-> the presubmit still using the checked-in SDK now treats those files as having pre-existing formatting issues and ignores their formatting.
* Bump the checked-in SDK as soon as the regular dev releases (Tue/Thu) pick up the dart_style change.
-> the presubmit and locally built SDKs are now in sync again.That would also ensure that all non-formatted files that somehow slipped through would be formatted whenever we roll dart_style.
WDYT?
SGTM. Thanks for pushing back on this. I wrote a process here:
https://github.com/dart-lang/dart_style/wiki/Release-process
How does that look?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +1 |
I reformatted the entire SDK, including tests. (At least, the tests that could be parsed. Tests with deliberate parse errors aren't formatted.) Reformatting tests can be dicey because sometimes they are testing behavior that is whitespace-specific. So to careful, I reviewed all the diffs (myself, not using AI 😅) to make sure the behavior didn't change.
Unless otherwise stated, the only difference is the result of running the formatter and also possibly updating static error comment markers (because the formatter changed the indentation).
The files that needed some special handling are:
I skipped formatting all of these. They have block comments whose location is significant for the test and formatting moves them. Someone who understands those tests better should ideally go back and format them. It might require wrapping some things in parentheses to get the block comments to stay where they are. Alternatively, you can put `// dart format off` at the top of each test to permanently disable formatting them. I just skipped them.
I disabled formatting this since it tests the lexer's behavior around whitespace.
I skipped formatting this. It contains trailing whitespace which the formatter removes but I believe is deliberate. (Annoyingly even with `// dart format off`, the formatter will remove trailing whitespace.)
These test how the parser handles a nul byte in the source code. The formatter does change the file, so I guess we can parse it. But my text editor and diff tool don't handle it so I can't see if the formatter did the right thing. To be safe, I skipped it.
Disabled formatting because these are testing parser edge cases where whitespace or trailing commas matter.
I moved some comments after formatting these.
I disabled formatting so that the "// Error." comments don't get moved.
I moved a semicolon to be more idiomatic.
I removed some unnecessary empty statements so that "// Error." comments don't get moved.
I turned some "///" non-doc comments into regular line comments so they don't get moved.
I disabled formatting because formatting moves a static error comment.
I disabled formatting because these are multitests.
I tweaked the static error expectation comments to work with the new formatting.
I disabled formatting because this hits a formatter bug (and added a link to the bug).
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Also, the test failures appear to be caused by this CL as well, PTAL at those before landing.
Yup, done now.
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
You may want to add
the 'CoreLibraryReviewExempt: only formatting' footer (to make the Core-Library-Review check happy)
Also, the 'Tested: <how this was tested>' footer for the Commit-Message-Has-TEST check.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
+Leaf for a global owner on my timezone. I already got +1 from Alex but I synced and merged from main today which touched a new file and cleared the +1.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// dart format offWould it be possible to mark entire folders with "dart format off"?
I would prefer that nothing under "pkg/front_end/parser_testcases/" was formatted.
// dart format offWould it be possible to mark entire folders with "dart format off"?
I would prefer that nothing under "pkg/front_end/parser_testcases/" was formatted.
I have been asked to clarify why I prefer this.
Bob has successfully found some tests that were deliberately written with a "weird" formatting. Kudos for that, that must have taken a while.
I wouldn't be able to say with any kind of certainty that all cases where the formatting was important at the time of test-writing has been found though.
With this being tests explicitly of the scanner/parser and how it interacts with the source code (including any additional of missing whitespace) I think it would better to leave it in whatever formatting it had.
// dart format offJens JohansenWould it be possible to mark entire folders with "dart format off"?
I would prefer that nothing under "pkg/front_end/parser_testcases/" was formatted.
I have been asked to clarify why I prefer this.
Bob has successfully found some tests that were deliberately written with a "weird" formatting. Kudos for that, that must have taken a while.
I wouldn't be able to say with any kind of certainty that all cases where the formatting was important at the time of test-writing has been found though.
With this being tests explicitly of the scanner/parser and how it interacts with the source code (including any additional of missing whitespace) I think it would better to leave it in whatever formatting it had.
SGTM. I'll undo the changes to everything in `parser_testcases/`.
Jens JohansenWould it be possible to mark entire folders with "dart format off"?
I would prefer that nothing under "pkg/front_end/parser_testcases/" was formatted.
Bob NystromI have been asked to clarify why I prefer this.
Bob has successfully found some tests that were deliberately written with a "weird" formatting. Kudos for that, that must have taken a while.
I wouldn't be able to say with any kind of certainty that all cases where the formatting was important at the time of test-writing has been found though.
With this being tests explicitly of the scanner/parser and how it interacts with the source code (including any additional of missing whitespace) I think it would better to leave it in whatever formatting it had.
SGTM. I'll undo the changes to everything in `parser_testcases/`.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +2 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +2 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +2 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roll the latest dart_style into the SDK.
This contains all of the formatting changes I intend to ship in Dart 3.13.
This same CL also reformats the entire SDK to match the new style. As soon as a new dev commit is available after this lands, I'll update the pre-built SDK to match that commit so that the presubmit uses the same style.
I also tidied up the CHANGELOG some and removed the examples since it was getting kind of verbose.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |