Skip to content

Conversation

@jason-simmons
Copy link
Member

No description provided.

@jason-simmons jason-simmons requested review from Hixie and flar September 1, 2020 19:10
@flar
Copy link
Contributor

flar commented Sep 1, 2020

Members of the infra team have other ideas, like not doing format testing during a customer test run. I'll wait for someone on that team to approve this or suggest another approach.

Tagging @christopherfujino

@christopherfujino
Copy link
Contributor

Members of the infra team have other ideas, like not doing format testing during a customer test run. I'll wait for someone on that team to approve this or suggest another approach.

Tagging @christopherfujino

I'm not part of the infra team, and I wasn't suggesting not landing this. However, I find the cocoon analyzer failure puzzling.

@christopherfujino christopherfujino requested review from keyonghan and removed request for christopherfujino September 1, 2020 19:18
@keyonghan
Copy link
Contributor

Cocoon has a test to enforce dartfmt --line-length=120..
https://github.com/flutter/cocoon/blob/master/test_utilities/bin/flutter_test_runner.sh#L26

@flar
Copy link
Contributor

flar commented Sep 1, 2020

Cocoon has a test to enforce dartfmt --line-length=120..
https://github.com/flutter/cocoon/blob/master/test_utilities/bin/flutter_test_runner.sh#L26

I don't believe this was failing the 120 character rule, the line that was reformatted was only 86 characters long.

Was there another change that happened other than line length?

Or perhaps there is a bug in the new dartfmt that ignored the requested 120 char length and went with its own 80 char length instead?

@jason-simmons
Copy link
Member Author

The Dart SDK team may have more info. But I suspect that dartfmt changed its rules for formatting nested conditional expressions.

@flar
Copy link
Contributor

flar commented Sep 1, 2020

I just applied this diff manually and reran the dartfmt command (with my local version that hasn't been updated) and it rejected the diff. This means that this change to progress_button.dart must be accompanied by the roll to the dartfmt command.

@flar
Copy link
Contributor

flar commented Sep 1, 2020

Here is the Dart change that is now enforcing this:

dart-lang/dart_style#945

@flar
Copy link
Contributor

flar commented Sep 1, 2020

I found that this formatting will pass the old formatter because I guess it wants to preserve comments:

+          onPressed: _busy // dartfmt will soon require this new formatting
+              ? null
+              : widget.onPressed != null // dartfmt will soon require this new formatting
+                  ? _handlePressed
+                  : null,

Oddly, aligning the comments made it complain because it wanted the comments to be right after the code they were commenting... :(

@flar
Copy link
Contributor

flar commented Sep 1, 2020

Suggestion from @jakemac53

Note that instead of running dartfmt from the sdk,  you _could_ globally activate the package and run it like `pub global run dart_style:format .`

that would allow you to run a consistent version (if you activated a specific version)

Copy link
Contributor

@keyonghan keyonghan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@flar
Copy link
Contributor

flar commented Sep 1, 2020

Have we double-checked that it passes the new formatter with the comments? I only ran the old formatter on it.

@jason-simmons
Copy link
Member Author

Yes - this works with the latest dartfmt

@flar flar merged commit 8e1da9f into flutter:master Sep 1, 2020
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

Successfully merging this pull request may close these issues.

4 participants