Pub.dev package suggestion: Consider adding a package to pub.dev with all lint rules

[rydmike]

Add and maintain all lint rules as a package on pub.dev

Currently a list of all lint rules is available as an automatically generated file here:
https://dart-lang.github.io/linter/lints/options/options.html

It would be useful if this lint rule list was also available as a package on pub.dev that is maintained and always up to date.
Developers could then use it to make their linting setup that first enables all available lint rules, and then in analysis options uses it and only turns of rules that are not desired.

Currently such setups requires first getting a copy of the all lint rules from https://dart-lang.github.io/linter/lints/options/options.html and putting it into a file, e.g. all_lint_rules.yaml that is used in the projct analysis_options.yaml file where not desired rules are turned off.

This works, but to keep up on new rules the developer has to check the page https://dart-lang.github.io/linter/lints/options/options.html for new versions regularly and update your own copy of it in the project.

A setup using this approach might look like this:

# Include and activate all lint rules, later below we disable the not used or desired ones.
# You can find a list of all lint rules to put in your all_lint_rules.yaml file here:
# https://dart-lang.github.io/linter/lints/options/options.html
include: all_lint_rules.yaml
analyzer:
  exclude:
    - "**/*.g.dart"
    - "**/*.freezed.dart"
    - "test/.test_coverage.dart"
    - "bin/cache/**"
    - "lib/generated_plugin_registrant.dart"

  strong-mode:
    implicit-casts: false
    implicit-dynamic: false

  errors:
    # Without ignore here, we cause import of all_lint_rules to warn, because some rules conflict.
    # We explicitly enabled even conflicting rules and are fixing the conflicts in this file.
    included_file_warning: ignore
    # Treat missing required parameters as an error.
    missing_required_param: error
    # Treat missing returns as an error, not as a hint or a warning.
    missing_return: error
    # Don't assign new values to parameters of methods or functions.
    # Treat assigning new values to a parameter as a warning.
    parameter_assignments: warning
    # Allow having TODOs in the code.
    todo: ignore

# LINTER Preferences
#
# Explicitly disable only the rules we do not want.
linter:
  rules:
    # ALWAYS separate the control structure expression from its statement.
    # This sometimes makes things more unclear when one line is enough.
    # Also single line `if`s are fine as recommended in Effective Dart "DO format your code using dartfmt".
    always_put_control_body_on_new_line: false

    # ALWAYS specify @required on named parameter before other named parameters.
    # Conflicts with the convention used by flutter, which puts `Key key`
    # and `@required Widget child` last.
    always_put_required_named_parameters_first: false
  
    # Followed by more turned OFF lint rules as preferred/needed/desired and always 
    # turning off at least conflicting rules.  ​
    ...

This kind of lint setup and its benefits is described eg here and here.

While this works fine, it would be very useful if the all lint rules list could be obtained and used as a dev dep package. With versions and change log, listing changes and new rules and links to their docs. It would make maintaining the list of all used rules in projects using this type of linting setup easier.

While anybody could create and manually keep such a package in sync with the automated html doc page. I think it would be preferable if the Dart team could create it and presumably use automated package CI/CD to keep such a pub.dev package with all lint rules in sync with the currently automatically generated html doc page.

[csells]

I agree that this would be an excellent way to get the benefit of all of the lints we build as we build them. @pq what do you think? is this doable?

[asidt]

I support this 100%. What @rydmike explains is also the approach I use in my side projects and production ones, and such a maintained package would be of great help for me!

[TimWhiting]

I also use this sort of setup and would love this.

[gaetschwartz]

As a (temporary?) workaround, I created this package you can use that automatically fetches the latest rules and generates the appropriate file.

https://github.com/gaetschwartz/all_lint_rules

You can use it like so:

dev_dependencies:
  all_lint_rules_community:
    git: https://github.com/gaetschwartz/all_lint_rules

and then include all.yaml like so in your analysis_options.yaml:

include: all_lint_rules_community/all.yaml

[pq]

@pq what do you think? is this doable?

Sounds reasonable to me.

The biggest question is where to host. On dart-lang does feel appropriate. And probably not in package:lints though I guess I'm open to it.

I'd be curious to get some input from ecosystem folks...

📟 @mit-mit @natebosch @jakemac53

also fyi @srawlins

[jakemac53]

My main concern would be the conflicting lints - I have a feeling we would end up fielding a lot of issues as people tried to use this file and got conflicting lints.

If we wanted to do this then instead of having the user deal with the conflicts I would probably just make an opinionated choice where there are conflicts.

[csells]

I agree with you, @pq, that we should not drop this into package:lints but create a new package.

@jakemac53 I agree that we should provide opinions in the sample analysis_options.yaml file we show in the README so that if folks simply copy&paste (which most will), they'll have a smooth experience.

[jakemac53]

I agree with you, @pq, that we should not drop this into package:lints but create a new package.

Can we elaborate a bit more on why we would want it in a separate package? That comes with a fair bit more overhead so we should outline some clear benefits.

[csells]

I don't want to interfere with the Dart and Flutter lints we shipped in Flutter 2.5. I want folks to be able to keep using those; these would be a new choice for all the lints. If we can make that happen in the same package, that's OK with me, too.

[pq]

Can we elaborate a bit more on why we would want it in a separate package?

My biggest concern is versioning. My gut says we want to keep versioning of the recommended and core rule sets distinct from something tracking the addition of new available lints. (And I should ask, where ever this lands, would we want to bump a version every time a new lint is added to all.yaml?)

[pq]

Having said that,

That comes with a fair bit more overhead so we should outline some clear benefits.

💯

If nothing else, we'd avoid having to bikeshed a new package name 😄

[jakemac53]

My biggest concern is versioning.

Do we intend on modeling this package with breaking versions any time we add a lint to one of the published sets, or modify the behavior of a lint such that it catches more cases?

If so then yes I would probably push back pretty hard on adding an "all" set of lints to this package.

[csells]

I think the only breaking change would be removing an existing lint.

[jakemac53]

I think the only breaking change would be removing an existing lint.

Removing a lint doesn't break any builds though - its adding lints or modifying them to trigger for more cases that breaks things.

[csells]

I'd argue that's a feature in this case.

[jakemac53]

Fwiw I am not arguing for treating these as breaking changes, but it sounded like we were considering doing that, and the answer would affect my comfort level with having a set of lints like this in this package. So we should clarify the policy at least (probably in the README).

You always have the option of pinning your lints package version, if you want to choose on which schedule you update your lints. I would recommend we tell users to do that if they don't want lints changing out from under them, as opposed to doing breaking changes in this package.

[csells]

sgtm

[srawlins]

Removing a lint doesn't break any builds though - its adding lints or modifying them to trigger for more cases that breaks things.

Yeah that would be my definition of a breaking change. Adding a new lint rule, or editing a lint rule to catch more cases, is a change that can turn any CI red, and should be considered a breaking change, and warrants a major version bump.

[jakemac53]

Yeah that would be my definition of a breaking change. Adding a new lint rule, or editing a lint rule to catch more cases, is a change that can turn any CI red, and should be considered a breaking change, and warrants a major version bump

Fwiw we do deprecations all the time and don't call those breaking, even though they turn CI red for many people. So we don't today use this definition. Adding or editing lints doesn't stop apps from compiling or running, and so I would personally not treat that as a breaking change, even though it can turn CI red for people. But they opted into making that breaking if that is the case.

[csells]

@jakemac53 agreed. also, anyone that opts into using all of the lints wants to be broken -- that's the whole point.

[srawlins]

My argument for the flutter and lints packages is that they do not include code that a user's app runs; the only output they produce is static error reports (in CI, IDEs), outside of app execution.

One motivation (the primary?) for SemVer is that all the devs, the CI, the release binary compiling machine, etc, all can rely on the version constraints to lead to the same outcome. The code should be statically compilable (no public API change) across environments, given one set of dependency version constraints. The code should lead to consistent static errors, given one set of version constraints. Tests should pass or fail consistently, given one set of version constraints.

This is especially true when any consumer of the linter or lints package has a strict "you cannot break us" requirement in their working relationship with the linter or lints package. Major versions let a consumer control what new and exciting lint rules will be pulled down by pub get, on a dev's machine, and on a CI bot trying out an emergency critical PR.

[jakemac53]

One motivation (the primary?) for SemVer is that all the devs, the CI, the release binary compiling machine, etc, all can rely on the version constraints to lead to the same outcome. The code should be statically compilable (no public API change) across environments, given one set of dependency version constraints. The code should lead to consistent static errors, given one set of version constraints. Tests should pass or fail consistently, given one set of version constraints.

These constraints are all upheld given the policy I outlined - because lints are not errors. You have to explicitly choose in your CI to fail on lints, so it is completely opt-in. Your app will always build/run regardless of lints - although your CI may fail if you have chosen it to.

This is especially true when any consumer of the linter or lints package has a strict "you cannot break us" requirement in their working relationship with the linter or lints package. Major versions let a consumer control what new and exciting lint rules will be pulled down by pub get, on a dev's machine, and on a CI bot trying out an emergency critical PR.

The concern is this ends up meaning that essentially all versions of the lints package are breaking changes. This overall leads to a worse situation, where users have to explicitly upgrade often, and ultimately they end up on outdated lint sets as a result. Either that or they use an any constraint but I think that is less than ideal.

The default case should instead be that users get the latest lints any time they pub upgrade - this means their CI may fail any time a new lint is added, but as @csells mentioned this is exactly the point. This is behavior most users should be opting into, especially external package authors. They want to know any time they are violating a new lint as soon as possible (it could affect their pub score, if nothing else).

Using this strategy it is also still trivial for users to opt to not have their CI be broken, they have two options:

• Don't fail CI on lints (in fact, this is the default)
• Pin the lints package to a specific version

[jamesderlin]

Should this issue really be distinct from #58029 and/or #58085?

I think in addition to moving all.yaml to a package, the included_file_warning: ignore behavior should be changed so that it emits warnings only about conflicting lints that remain enabled.

[rydmike]

@jamesderlin agreed, it would be a nice improvement if warning about conflicting rules was only triggered after final effective result is known. When using the above setup, if there are new rules in the include with all lint rules, I now sometimes temporary change included_file_warning: ignore to warning ,to check that there are no new net result conflicts, or other errors in included file.

If it would only warn about conflicting rules after net effect in include and the rules disabled in analysis_options.yaml it would be helpful.