Style Guide updates #152525

nate-thegrate · 2024-07-30T00:28:01Z

This pull request contains various updates for the the style guide:

Removes the Prefer single quotes for strings and Separate the 'if' expression from its statement sections, since at this point, any contributions that violate these guidelines will trigger linter rules.
Updates examples in the style guide based on style guidelines (i.e. previously some guidelines were contradicted by code samples in the file). This includes if-statement curly braces, aligning expressions, and trailing commas.
Alters the => rule to include multi-line getters that return collection literals or switch expressions. (This is based on a recommendation from Bernardo Ferrari and Michael Goderbauer in a previous comment thread.)

nate-thegrate · 2024-07-30T00:32:34Z

docs/contributing/Style-guide-for-Flutter-repo.md

-  if (_theProperty == value)
+  if (_theProperty == value) {
    return;
+  }


Now that we've enabled a linter rule to enforce if-statement curly braces, it makes sense for the examples here to reflect that.

nate-thegrate · 2024-07-30T00:33:27Z

docs/contributing/Style-guide-for-Flutter-repo.md

-      if (owner.debugDoingLayout)
-        return (RenderObject.debugActiveLayout == parent) &&
-            parent.debugDoingThisLayout;
-      if (owner.debugDoingPaint)
-        return ((RenderObject.debugActivePaint == parent) &&
-                parent.debugDoingThisPaint) ||
-            ((RenderObject.debugActivePaint == this) && debugDoingThisPaint);
+      if (owner.debugDoingLayout) {
+        return (RenderObject.debugActiveLayout == parent) && parent.debugDoingThisLayout;
+      }
+      if (owner.debugDoingPaint) {
+        return ((RenderObject.debugActivePaint == parent) && parent.debugDoingThisPaint)
+            || ((RenderObject.debugActivePaint == this)   && debugDoingThisPaint);
+      }


This change is based on Align expressions; it also adds if-statement curly braces.

nate-thegrate · 2024-07-30T00:37:10Z

docs/contributing/Style-guide-for-Flutter-repo.md

-However, where possible avoid global constants. Rather than `kDefaultButtonColor`, consider `Button.defaultColor`. If necessary, consider creating a class with a private constructor to hold relevant constants.
+However, where possible avoid global constants. Rather than `kDefaultButtonColor`, consider `Button.defaultColor`. If necessary, consider creating an `abstract final class` to hold relevant constants.


Classes built to hold constants (e.g. Colors and Durations) now use the abstract and final modifiers in place of a private constructor.

nate-thegrate · 2024-07-30T00:41:32Z

docs/contributing/Style-guide-for-Flutter-repo.md

 ```dart
-  setState(() { /* The animation ticked. We use the animation's value in the build method. */ });
+  setState(() {
+    // The animation ticked. We use the animation's value in the build method.
+  });
 ```


This change is based on the 80 characters guideline, along with the Dart documentation guide linked in the Documentation section:

DON'T use block comments for documentation

void greet(String name) { // Assume we have a valid name. print('Hi, $name!'); }

void greet(String name) { /* Assume we have a valid name. */ print('Hi, $name!'); }

nate-thegrate · 2024-07-30T00:43:10Z

docs/contributing/Style-guide-for-Flutter-repo.md

 // one-line constructor example
-abstract class Foo extends StatelessWidget {
-  Foo(this.bar, { Key key, this.child }) : super(key: key);
-  final int bar;
-  final Widget child;
+class ConstantTween<T> extends Tween<T> {
+  ConstantTween(T value) : super(begin: value, end: value);
+
  // ...
 }


The old example would have triggered the use_super_parameters rule (I'm sure it was written before that feature existed).

nate-thegrate · 2024-07-30T00:56:05Z

docs/contributing/Style-guide-for-Flutter-repo.md

-### Prefer single quotes for strings
-
-Use double quotes for nested strings or (optionally) for strings that contain single quotes.
-For all other strings, use single quotes.
-
-Example:
-
-```dart
-print('Hello ${name.split(" ")[0]}');
-```


This section was likely written before the prefer_single_quotes and avoid_escaping_inner_quotes rules were introduced.

Since violations of this guideline result in analysis check failures, there isn't any additional utility that we gain from keeping this section. Removing it decreases the total length of this markdown file, making it more likely that the reader will pay attention to other sections.

nate-thegrate · 2024-07-30T00:57:20Z

docs/contributing/Style-guide-for-Flutter-repo.md

          return <PopupMenuItem<String>>[
            PopupMenuItem<String>(
              value: 'Friends',
-              child: MenuItemWithIcon(Icons.people, 'Friends', '5 new')
+              child: MenuItemWithIcon(Icons.people, 'Friends', '5 new'),
            ),
            PopupMenuItem<String>(
              value: 'Events',
-              child: MenuItemWithIcon(Icons.event, 'Events', '12 upcoming')
+              child: MenuItemWithIcon(Icons.event, 'Events', '12 upcoming'),
            ),
          ];


Updated based on the Use a trailing comma guideline.

nate-thegrate · 2024-07-30T01:01:18Z

docs/contributing/Style-guide-for-Flutter-repo.md

-### Separate the 'if' expression from its statement
-
-(This is enforced by the `always_put_control_body_on_new_line` and `curly_braces_in_flow_control_structures` lints.)


Removing this section, for the same reason as 'Prefer single quotes' above.

nate-thegrate · 2024-07-30T01:24:46Z

docs/contributing/Style-guide-for-Flutter-repo.md

-  foo(bar,
-    baz);


This is the only change I'm on the fence about.

Personally, I think the other two examples are much easier to visually parse than this one.

Why are you on the fence with this one ? This would only make sense if it breaks the max char per line rule, wouldn't it ?

If foo() takes up multiple lines due to a callback/switch expression/list or map literal, then I think it's all right to format it this way.

foo(bar, () { baz.quux(); return baz.toInt(); }); foo(bar, switch (baz) { 1 => 'a', 2 => 'b`, _ => `c`, }); foo(bar, [ 'quux', 'qaax', 'qeex', ]);

But in any other situation where the foo() arguments take up more than 1 line, I think it's much more readable with a trailing comma.

foo( bar, baz, );

I think it'd be nice to get another perspective on this, and then possibly add the above examples to this section.

I agree with removing this example and would be fine with adding the other examples you're mentioning in the comment above.

Awesome!

Examples have been added in the most recent commit. I'll plan on adding autosubmit tomorrow, to leave a window for any additional feedback 👍

goderbauer

LGTM

goderbauer · 2024-08-15T22:12:21Z

docs/contributing/Style-guide-for-Flutter-repo.md

-  foo(bar,
-    baz);


I agree with removing this example and would be fine with adding the other examples you're mentioning in the comment above.

nate-thegrate · 2024-08-16T01:00:55Z

docs/contributing/Style-guide-for-Flutter-repo.md

-  foo(bar,
-    baz);


Awesome!

Examples have been added in the most recent commit. I'll plan on adding autosubmit tomorrow, to leave a window for any additional feedback 👍

nate-thegrate · 2024-08-16T01:08:47Z

docs/contributing/Style-guide-for-Flutter-repo.md

-closing `]`, `}`, or `)` bracket will line up with the argument name, for
-named arguments, or the `(` of the argument list, for positional arguments.


I was confused by "argument name" and "argument list" here, since the code has a nested argument structure.

nate-thegrate · 2024-08-16T01:11:36Z

docs/contributing/Style-guide-for-Flutter-repo.md

+you can instead use the `=>` form.\
+When doing this, the closing `]`, `}`, or `)` bracket will have the same
+indentation as the line where the callback starts.


I picked up the \ from the resources linked in the semantic line breaks documentation :)

goderbauer

LGTM

flutter/flutter@bced008...a0c0453 2024-08-16 [email protected] Style Guide updates (flutter/flutter#152525) 2024-08-16 [email protected] Roll Packages from 86d15a6 to 2c37fb0 (5 revisions) (flutter/flutter#153571) 2024-08-16 [email protected] Move Android API level 35 emulator tests to staging (flutter/flutter#153568) 2024-08-16 [email protected] Roll Flutter Engine from a8fefc81188e to d5bf3afc601f (2 revisions) (flutter/flutter#153565) 2024-08-16 [email protected] [Swift Package Manager] Test removing the last Flutter plugin (flutter/flutter#153519) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This pull request updates the style guide for improved internal consistency and to match current linter rules. I'll make comments for each change here!

github-actions bot added c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. d: docs/ flutter/flutter/docs, for contributors labels Jul 30, 2024

nate-thegrate commented Jul 30, 2024

View reviewed changes

nate-thegrate force-pushed the style-guide-updates branch from 180f5d3 to e203e1a Compare July 30, 2024 01:02

nate-thegrate commented Jul 30, 2024

View reviewed changes

nate-thegrate marked this pull request as ready for review July 30, 2024 01:25

goderbauer approved these changes Aug 15, 2024

View reviewed changes

nate-thegrate commented Aug 16, 2024

View reviewed changes

nate-thegrate added 2 commits August 15, 2024 22:31

style guide updates

bec9186

clarify wording for list/map literals & switch expressions

29f409c

nate-thegrate force-pushed the style-guide-updates branch from 0f4bda6 to 29f409c Compare August 16, 2024 04:31

nate-thegrate added 2 commits August 15, 2024 22:40

fix typo

c334d63

const constructors :)

666a80f

goderbauer approved these changes Aug 16, 2024

View reviewed changes

nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 16, 2024

auto-submit bot merged commit a0c0453 into flutter:master Aug 16, 2024

nate-thegrate deleted the style-guide-updates branch August 16, 2024 16:19

engine-flutter-autoroll mentioned this pull request Aug 16, 2024

Roll Flutter from bced008679af to a0c04534f5ea (5 revisions) flutter/packages#7429

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2024

a0c0453 Style Guide updates (flutter/flutter#152525)

236d19e

DBowen33 pushed a commit to DBowen33/flutter that referenced this pull request Aug 16, 2024

Style Guide updates (flutter#152525)

851005e

This pull request updates the style guide for improved internal consistency and to match current linter rules. I'll make comments for each change here!

Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024

Style Guide updates (flutter#152525)

ed28dbf

This pull request updates the style guide for improved internal consistency and to match current linter rules. I'll make comments for each change here!

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024

a0c0453 Style Guide updates (flutter/flutter#152525)

7e60832

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024

a0c0453 Style Guide updates (flutter/flutter#152525)

7cb7f2a

		However, where possible avoid global constants. Rather than `kDefaultButtonColor`, consider `Button.defaultColor`. If necessary, consider creating a class with a private constructor to hold relevant constants.
		However, where possible avoid global constants. Rather than `kDefaultButtonColor`, consider `Button.defaultColor`. If necessary, consider creating an `abstract final class` to hold relevant constants.

		### Separate the 'if' expression from its statement

		(This is enforced by the `always_put_control_body_on_new_line` and `curly_braces_in_flow_control_structures` lints.)

		closing `]`, `}`, or `)` bracket will line up with the argument name, for
		named arguments, or the `(` of the argument list, for positional arguments.

Style Guide updates #152525

Style Guide updates #152525

Uh oh!

Conversation

nate-thegrate commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DON'T use block comments for documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cedvdb Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nate-thegrate commented Jul 30, 2024 •

edited

Loading

cedvdb Jul 31, 2024 •

edited

Loading