- Notifications
You must be signed in to change notification settings - Fork28.9k
Looking at ways we aren't actually following the style guide#172296
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
| refactoring code much harder. (These are commonly used in Flutter's codebase today, but that is almost | ||
| always a mistake. When you are editing a file that uses those features, aim to reduce the number of | ||
| tests using them while you're there.) | ||
| Avoid using test-global variables or other state shared between tests. They make writing tests easier |
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.
ThesetUp andtearDown methods are used in many places currently.
Should we fix that? Or remove this specification from the style guide?
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.
I'm a big believer in stateless or near-stateless test fixtures, and have been chipping away at the pattern in flutter/packages. I often point people tohttps://abseil.io/tips/122 in reviews (I forgot we had this here).
I still think this is good advice, personally (and will continue to advocate for it even if it's not in the guide).
| Specifically, we are trying to avoid shared state, which could persist across tests, and non-local | ||
| side-effects, which would prevent being able to move a test to another file without breaking the test. | ||
| (It's fine to factor out code into functions that are called by tests, so long as the functions don't | ||
| have side-effects that might change how other tests run.) | ||
| ### Prefer more test files, avoid long test files |
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.
In packages/flutter alone, 528 test files (out of 942) are over 200 lines in length.
The average line length for a test file in package/flutter is 1,438 lines, so we are clearly not following this.
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.
We could adjust this rule to a more realistic line length, just remove it, or break up test files (which would be a lot of them).
| suite. (It also makes developing the tests faster because you can run the test file faster.) | ||
| ### Avoid using `pumpAndSettle` |
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.
This is being used in multiple places.
Should we remove usage? Or remove the rule?
| @@ -1075,17 +1060,6 @@ when selecting a name for an identifier. In general, avoid one-character names u | |||
| (for example, prefer `index` over `i`, but prefer `x` over `horizontalPosition`). | |||
| ### Avoid anonymous parameter names | |||
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.
Also being used in multiple places.
Should we remove usage or remove the rule?
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.
Dart supporting wildcard variables now is something newer that we may be showing off as well.
| @@ -1312,118 +1286,7 @@ Line length for code is automatically handled by `dart format`, which is configu | |||
| line length of 100. | |||
| ### Consider using `=>` for short functions and methods | |||
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.
This and the following rule make for a complex decision around using=>. See following comment for full suggested adjustment.
(This rule and output from Gemini in flutter/packages prompted this audit 🙂 -flutter/packages#9627 (comment))
| ### Use braces for long functions and methods | ||
| ### Use braces for long functions and methods instead of `=>` |
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.
Proposal: Keep it simple.
We already have several violations of existing rules, most likely due to formatter.
| ### Use braces for long functions and methods | ||
| ### Use braces for long functions and methods instead of `=>` | ||
| Use a block (with braces) when a body would wrap onto more than one line (as opposed to using `=>`; the cases where you can use `=>` are discussed in the previous two guidelines). |
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.
If we remove the above, this line would need to change.
Uh oh!
There was an error while loading.Please reload this page.
Prompted by Gemini output that has been given our style guide:flutter/packages#9627 (comment)
Started looking at
=>, but also looked to see where else we may be out of compliance with the style guide - or the style guide may not longer be correct. Feedback welcome. :)Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel onDiscord.