-
Notifications
You must be signed in to change notification settings - Fork 100
Update app_flutter to match the latest dartfmt rules #931
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
Conversation
|
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. |
7c20520 to
a274344
Compare
|
Cocoon has a test to enforce |
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? |
|
The Dart SDK team may have more info. But I suspect that dartfmt changed its rules for formatting nested conditional expressions. |
|
I just applied this diff manually and reran the |
|
Here is the Dart change that is now enforcing this: |
|
I found that this formatting will pass the old formatter because I guess it wants to preserve comments: Oddly, aligning the comments made it complain because it wanted the comments to be right after the code they were commenting... :( |
|
Suggestion from @jakemac53 |
keyonghan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
Have we double-checked that it passes the new formatter with the comments? I only ran the old formatter on it. |
|
Yes - this works with the latest dartfmt |
No description provided.