Skip to content

Conversation

@goderbauer
Copy link
Member

@goderbauer goderbauer commented May 6, 2021

Pre-work for this PR is done:

@google-cla google-cla bot added the cla: yes Contributor has signed the Contributor License Agreement label May 6, 2021
@goderbauer goderbauer marked this pull request as draft May 6, 2021 21:59
@goderbauer
Copy link
Member Author

FYI @pq @mit-mit @devoncarew @csells

## Summary

The [`package:flutter_lints`][] defines the latest set of recommended lints that
encourage good coding practises for Flutter apps, packages, and plugins. Projects
Copy link
Contributor

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".

Copy link
Contributor

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

Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

=>

Dart universe.

?

Copy link
Contributor

@pq pq left a 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.
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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:`
Copy link
Member

Choose a reason for hiding this comment

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

s/dart/Dart/

Copy link
Contributor

@sfshaza2 sfshaza2 left a 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>
Copy link
Contributor

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?

Copy link
Member Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[`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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[`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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[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
Copy link
Contributor

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...

Copy link
Member Author

@goderbauer goderbauer May 10, 2021

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.

Copy link
Contributor

@sfshaza2 sfshaza2 left a 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...

@goderbauer
Copy link
Member Author

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.

@sfshaza2
Copy link
Contributor

SG

@goderbauer goderbauer marked this pull request as ready for review May 19, 2021 00:00
@goderbauer
Copy link
Member Author

Added the missing version info. This PR is ready now!

Copy link
Contributor

@pq pq left a comment

Choose a reason for hiding this comment

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

Nice doc!

@goderbauer goderbauer requested a review from sfshaza2 May 19, 2021 17:19
Copy link
Contributor

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

LGTM

@sfshaza2 sfshaza2 merged commit 5fbb57b into flutter:master May 19, 2021
@goderbauer goderbauer deleted the lints branch May 19, 2021 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Contributor has signed the Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants