-
Notifications
You must be signed in to change notification settings - Fork 171
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
Pub.dev package suggestion: Consider adding a package to pub.dev with all lint rules #2994
Comments
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? |
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! |
I also use this sort of setup and would love this. |
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 include: all_lint_rules_community/all.yaml |
Sounds reasonable to me. The biggest question is where to host. On I'd be curious to get some input from ecosystem folks... 📟 @mit-mit @natebosch @jakemac53 also fyi @srawlins |
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. |
I agree with you, @pq, that we should not drop this into @jakemac53 I agree that we should provide opinions in the sample |
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. |
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. |
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 |
Having said that,
💯 If nothing else, we'd avoid having to bikeshed a new package name 😄 |
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. |
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. |
I'd argue that's a feature in this case. |
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 |
sgtm |
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. |
@jakemac53 agreed. also, anyone that opts into using all of the lints wants to be broken -- that's the whole point. |
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 |
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.
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 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:
|
@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 If it would only warn about conflicting rules after net effect in |
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 projctanalysis_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:
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.
The text was updated successfully, but these errors were encountered: