-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Migration guide for package:flutter_lints #5749
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
| ## Summary | ||
|
|
||
| The [`package:flutter_lints`][] defines the latest set of recommended lints that | ||
| encourage good coding practises for Flutter apps, packages, and plugins. Projects |
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 think this should be "practices".
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.
Correct, we use American English, not British English on our developer sites: https://developers.google.com/style/spelling?hl=en. :D
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.
Yes, definitely! We use American English on our developer sites.
|
|
||
| ## Context | ||
|
|
||
| Prior to the introduction of `package:flutter_lints` the Flutter framework |
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.
=>
Prior to the introduction of package:flutter_lints,
?
| used by the [dart analyzer][] to identify code issues if a Flutter project | ||
| didn't define a custom `analysis_options.yaml` file. Since `analysis_options_user.yaml` | ||
| was tied to a particular framework version, it was difficult to evolve without | ||
| breaking existing apps, packages, and plugins. As a result of that the lints |
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.
=>
As a result of that,
?
| `package:flutter_lints` was created. The package versions the lint set to enable | ||
| evolving it without breaking existing projects. Since the package builds on | ||
| Dart's [`package:lints`][] it also aligns the lints recommended for Flutter | ||
| projects with the rest of the dart universe. |
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 universe.
?
pq
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.
In general, looks great. Nice docs!
A few super minor nits...
| `package:flutter_lints` was created. The package versions the lint set to enable | ||
| evolving it without breaking existing projects. Since the package builds on | ||
| Dart's [`package:lints`][] it also aligns the lints recommended for Flutter | ||
| projects with the rest of the Dart universe. |
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.
Suggest s/universe/ecosystem/ which sounds a bit friendlier?
| include: package:flutter_lints/flutter.yaml | ||
| ``` | ||
| The newly activated lint set may identify some previously undiscovered issues in |
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.
s/some previously undiscovered/new/ ?
| ``` | ||
| The newly activated lint set may identify some previously undiscovered issues in | ||
| your code. To find them, run `flutter analyze` in the root directory of your |
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.
Perhaps also mention that you can simply open the project in an IDE with Dart support?
| the lints from `package:flutter_lints`. If your `analysis_options.yaml` already | ||
| contains an `include:` directive you have to decide whether you want to keep | ||
| those lints or whether you want to replace it with the lints from | ||
| `package:flutter_lints` because the dart analyzer only supports one `include:` |
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.
s/dart/Dart/
sfshaza2
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.
Nice doc, but I am wondering if we should move it.
|
|
||
| ## Timeline | ||
|
|
||
| Landed in version: 2.xx<br> |
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.
Do you know which version?
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.
Not yet. I'll update this PR as soon as I know.
| [Add flutter_lints package]: {{site.github}}/flutter/packages/pull/343 | ||
| [`analysis_options_user.yaml`]: {{site.github}}/flutter/flutter/blob/master/packages/flutter/lib/analysis_options_user.yaml | ||
| [Customizing static analysis]: https://dart.dev/guides/language/analysis-options | ||
| [dart analyzer]: https://dart.dev/guides/language/analysis-options |
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 analyzer]: https://dart.dev/guides/language/analysis-options | |
| [dart analyzer]: {{site.dart-site}}/guides/language/analysis-options |
| [dart analyzer]: https://dart.dev/guides/language/analysis-options | ||
| [Integrate package:flutter_lints into template]: {{site.github}}/flutter/flutter/pull/81417 | ||
| [Issue 78432 - Update lint set for Flutter applications]: {{site.github}}/flutter/flutter/issues/78432 | ||
| [`package:flutter_lints`]: https://pub.dev/packages/flutter_lints |
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.
| [`package:flutter_lints`]: https://pub.dev/packages/flutter_lints | |
| [`package:flutter_lints`]: {{site.pub}}/packages/flutter_lints |
| [Integrate package:flutter_lints into template]: {{site.github}}/flutter/flutter/pull/81417 | ||
| [Issue 78432 - Update lint set for Flutter applications]: {{site.github}}/flutter/flutter/issues/78432 | ||
| [`package:flutter_lints`]: https://pub.dev/packages/flutter_lints | ||
| [`package:lints`]: https://pub.dev/packages/lints |
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.
| [`package:lints`]: https://pub.dev/packages/lints | |
| [`package:lints`]: {{site.pub}}/packages/lints |
| [Issue 78432 - Update lint set for Flutter applications]: {{site.github}}/flutter/flutter/issues/78432 | ||
| [`package:flutter_lints`]: https://pub.dev/packages/flutter_lints | ||
| [`package:lints`]: https://pub.dev/packages/lints | ||
| [Package dependencies]: https://dart.dev/tools/pub/dependencies |
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.
| [Package dependencies]: https://dart.dev/tools/pub/dependencies | |
| [Package dependencies]: {{site.dart-site}}/tools/pub/dependencies |
| * [ThemeData's accent properties have been deprecated][] | ||
|
|
||
| [Default Scrollbars on Desktop]: /docs/release/breaking-changes/default-desktop-scrollbars | ||
| [Introducing package:flutter_lints]: /docs/release/breaking-changes/flutter-lints-package |
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.
Hmmm... It seems that we should document this in the "main" portion of the site, rather than bury it in breaking changes, where it can only be found via google search...
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 am open to that. :) Do you have a place in mind?
Also, we will link to this page from a tooling message when it is discovered that your setup is out of date.
sfshaza2
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.
On second thought, I'm ok with this living in the breaking changes docs...
|
I am now just waiting on a Flutter dev release that contains flutter/flutter#81417 to obtain the version number that needs to be included in this guide. |
|
SG |
|
Added the missing version info. This PR is ready now! |
pq
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.
Nice doc!
sfshaza2
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
Pre-work for this PR is done: