Repository navigation
☂️ Update lint set for Flutter applications #78432
Description
Activity
One comment:
core.yaml, recommend.yaml, and potentially a Flutter-specific lint file are published in a package
If we do decide that we need a Flutter specific lint file (if we're not able to in-line everything into core and recommended), the flutter specific lints could just be shipped as part of the flutter package (e.g., packages/flutter/lib/flutter_recommended.yaml).
I believe that would cause versioning issues: Whenever you upgrade your version of flutter you'd be forced to also upgrade your code base to the latest lints in
packages/flutter/lib/flutter_recommended.yaml. If it ships as part of a separate (versioned) package, updating flutter and updating the set of recommended lints can happen independently.It would definitely be a decision in terms of how we want these to roll out. We could version them with the sdk and ship changes as part of new stable versions.
/fyi also @davidmorgan @bwilkerson
Thanks everyone!
Commented on the doc re: versioning.
package_pedanticsolves this by having versioned files within the package, so everyone can always be on the latest version and still choose which version they want. Additionally, there is one file that always includes the latest lints, in case you want to auto update rather than pin.That model seems to work :)
It is not clear to me why you'd need the double versioning (pub package version + yaml version). If the package doesn't contain any code, wouldn't it be enough to just use the package version? Whenever a new lint is added, it's treated as a breaking change and the MAJOR version number is bumped. If you're ready to upgrade to the latest set of recommended lints (and fix the warnings produced by it), you bump the package version in you package's pubspec.
Reacted by Michael ThomsenSorry, my mistake--I checked: the analyzer has no problems resolving different yaml versions from different packages. Please ignore :)
The reason for double-versioning is that it allows you to not use the most recent version of the lints.
That's not important if the lints package is only a dev-dependency for all packages. If any package you depend on uses the lints package as a non-dev-dependency, you'll need to use a version accepted by that other package.
If we make a change to the lints file, by adding a lint, your code might no longer be lint-warning-free. Some build systems might consider that an error. That means that you need to migrate your code to the new lints, and until then, you might want to keep your lints at the previous version.
Again, if everybody uses the lints package as a dev-dependency only, you can just lock yourself to an older version of the lints package. If not, you might need to depend on a newer version of the lints package, and the versioned yaml files allows you to still use the older lints file.
(Arguably, you can also just add
the_new_lint: ignoretoanalysis_options.yamlto override the import, but that requires more work for the user to figure out which precise lints have changed between versions).The above was assuming that the package is added as a dev-dependency, which I think is the right thing to do for this package. If you don't do that correctly, it will cause problems, yes. But that's also true for other dev-dependencies that were accidentally added as a regular dependency, no? E.g. if I accidentally add package:test as a regular dependency I am also forcing the hands of other packages that depend on me...
Reacted by Phil QuitslundWe know from
pedanticthat making it a full dependency is a bad idea ;) we don't have an example for publishing yaml in a dev dependency always working well, but as far as I can tell it should :)Can we put something in a package's
pubspec.yamlthat would make it impossible to include as a regular (non-dev) dependency?No such thing exists, but we could introduce it.
I think it's a bit strong to say "never". Perhaps it's sufficient to introduce something that:
- Changes
dart pub add <package>to add it asdev_dependency - Produces a warning in
dartanalyzer.
Maybe it's just as simple as having
is_dev_dependency: true, we can always deprecate such a feature without side-effects.
The downside of completely forbidding consumption as non-dev dependency is that any package intended to be used as a dev-dependency could certainly be used by another tool that is itself a dev-dependency.
Reacted by Phil Quitslund, Michael Thomsen and Bob NystromReacted by Phil Quitslund- Changes
Maybe it's just as simple as having is_dev_dependency: true, we can always deprecate such a feature without side-effects.
FWIW, I really like this proposal. 👍
SGTM.
Prior art for is_dev_dependency: bazel's testonly attribute (no direct link, sorry, please search that table). Note that would suggest that an
is_dev_dependencypackage can use otheris_dev_dependencypackages as a full dependency.So e.g.
package:testcan be markedis_dev_dependency, and mypackage:more_awesome_testcan be markedis_dev_dependencyand depend onpackage:testas a full dependency.This is a big step beyond the simple non-transitive version we need for lints yaml, but maybe worth a thought :) could be added to the simple version later, so doesn't need to block anything.
Updated the top comment with the current status.
@goderbauer I created tracking issues for the punted lints in the
package:lintsrepo:
https://github.com/dart-lang/lints/issues?q=is%3Aissue+is%3Aopen+label%3Atype-lintReacted by Michael GoderbauerThe main goal of this issue has been achieved, a migration guide for existing projects is published at https://flutter.dev/docs/release/breaking-changes/flutter-lints-package.
What's left to do is to update DartPad (tracked in dart-lang/dart-pad#1881) and actively nudging existing/established projects to upgrade to
pkg:flutter_lints(this is tracked in #82948).I am going to close this umbrella issue since the main work is done and the remaining auxiliary work is tracked separately.
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of
flutter doctor -vand a minimal reproduction of the issue.- locked as resolved and limited conversation to collaborators
on Aug 1, 2021
The lints currently applied by
flutter analyzeto flutter applications created withflutter createis outdated:analysis_options_user.yamlreceived its last meaningful update in October 2019. This umbrella issue tracks the work necessary to update those lints and align the set of lints recommended for Flutter application with the Dart universe.Details of the approach are discussed in http://flutter.dev/go/flutter-lints.
Work Items
avoid_private_typedef_functionsfromcore.yamlomit_local_variable_typeslint (Update: Removed fromrecommend.yaml)core.yaml,recommend.yaml, and potentially a Flutter-specific lint file are published in a packageflutter createto generate ananalysis_options.yamlfile importing the new recommended set of lints from that package (RFC: Integrate package:flutter_lints into templates #81417)analysis_options_user.yamlwith a message (duringflutter analyze) to upgrade to a customanalysis_options.yamlfile importing the new recommended set of lintsPunted
We've decided to remove
unawaited_futuresandprefer_mixinsfrom the lint set for now so the following issues are no longer blocking and can be addressed later.unawaited_futureslint is ready for Flutter code:NavigatorandAnimationControllermethods, maybe others)prefer_mixinslint is ready for Flutter code:ChangeNotifierandWidgetsBindingObserver, enable lint in flutter codebase for dogfooding/cc @pq @Hixie @mit-mit @munificent @devoncarew I think these are the work items that came out of our meeting the other day.