Skip to content
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

proposal: remove lints deprecated for 3.0 #95

Closed
2 tasks
Tracked by #50707
pq opened this issue Dec 14, 2022 · 25 comments
Closed
2 tasks
Tracked by #50707

proposal: remove lints deprecated for 3.0 #95

pq opened this issue Dec 14, 2022 · 25 comments

Comments

@pq
Copy link
Member

pq commented Dec 14, 2022

dart-lang/linter#3879 tracks issues that are being discussed for deprecation in 3.0.

Currently 4 3 of them are core/recommended.

There's an open question about null_closures so that may come off the list. (EDIT: yes.)

Two things I'd like to establish:

  • Agreement. Do we agree with this list of deprecations? (Are there more?)
  • Mechanics. Having agreed, we should settle on how we want to do a new package:lints release. (Do we want to have version scoped releases? That is, a release with an SDK lower bound of 3.0.0 or similar?)

/cc @mit-mit @munificent @lrhn @natebosch @jakemac53 @goderbauer for feedback

/fyi @jacob314 @bwilkerson @srawlins

@pq pq added the type-lint label Dec 14, 2022
@srawlins
Copy link
Member

null_closures is still necessary in Dart 3.0. For example, Iterable.firstWhere's orElse parameter is nullable.

@pq
Copy link
Member Author

pq commented Jan 6, 2023

Regarding mechanics, how about a 2.1.0 release with lints removed and a new lower bound like this:

   sdk: '>=3.0.0-0 <4.0.0'

(EDIT: talking to @srawlins, we may want a release before that includes enable_null_safety (assuming we agree on #97?)

(EDIT 2: see below for a new proposal that does not bump the lower bound.)

@srawlins
Copy link
Member

srawlins commented Jan 6, 2023

I don't think there is anything breaking about removing lints from lists. I think that could be 2.1.0, and the SDK does not need to be bumped. It can stay at sdk: '>=2.17.0 <3.0.0 in order to benefit people already running on null safety.

Perhaps that is not safe because they could have opted-out files, running Dart 2.17, and those opted out files would no longer receive the benefit of those lint rules.

@pq
Copy link
Member Author

pq commented Jan 27, 2023

EDIT: new proposal.

Having thought about this some more and talking it over with @bwilkerson at length, I have a new simpler proposal:

A 2.1.0 release with lints removed and no SDK lower bound changes. (SDK lower bound is not really want we want to predicate removing lints on anyway -- what makes them irrelevant is the library language version and if you can still opt back to pre 2.12, the SDK lower bound won't work as intended.)

Risk. There is a risk that folks who have opted libraries to pre-2.12 will lose some coverage. (I expect this to be extremely uncommon in practice.)

How do folks feel?

/cc @mit-mit @munificent @lrhn @natebosch @jakemac53 @goderbauer @jacob314 @bwilkerson @srawlins

@srawlins
Copy link
Member

Would this be after we finish the internal migration? E.g. always_require_non_null_named_parameters is used internally.

@bwilkerson
Copy link
Member

Is it enabled by including one of these lint sets? If so, then we'd need to enable it independently of these lint sets before removing it from the lint set. If not, then there's no reason to wait.

@srawlins
Copy link
Member

srawlins commented Jan 27, 2023

Haha good question Brian. Total brain fart. We don't use these lint sets so just ignore me. 😄 No problem for internal use.

@jakemac53
Copy link

jakemac53 commented Jan 27, 2023

Have we considered just making the lints themselves be no-ops based on language version? It seems to me that they will still have value for a long time given packages may be on an old language version for quite some time (O(years)?).

@bwilkerson
Copy link
Member

Yes. But we want to let people know that they can remove them from their analysis options files just to help keep the world a little cleaner. But we can't let them know if they're still in the core, recommended, or flutter sets because then there would be too much noise. Hence the desire to remove them from these sets.

@pq
Copy link
Member Author

pq commented Jan 27, 2023

@jakemac53 that's essentially what we've been doing but to Brian's point there's some value in tidying up. Also, to be super clear, these lints are only valuable in libraries that opt back pre 2.12 which we're really discouraging in >=3.0.

@goderbauer
Copy link
Contributor

goderbauer commented Jan 27, 2023

The suggestion in #95 (comment) SGTM. Presumably, we'd also do a 2.1.0 release of flutter_lints that just bumps the package:lints version to 2.1.0? Are there any additional lints in flutter_lints that have become obsolete and should be removed as well? The list of those lints is here: https://github.com/flutter/packages/blob/main/packages/flutter_lints/lib/flutter.yaml. I believe these are all still relevant even with Dart 3.0. Just double-checking...

@goderbauer
Copy link
Contributor

Actually, flutter_lints already specifies lints: ^2.0.0 in its pubspec, so it would pick up a 2.1.0 release of package:lint already. Should we still do a release to bump that dependency to 2.1.0 explicitly?

@pq
Copy link
Member Author

pq commented Jan 27, 2023

@goderbauer: I think all the flutter-specific lints are good. As for a version bump, I don't know if it's necessary but don't feel really strongly either way!

@jakemac53
Copy link

Yes. But we want to let people know that they can remove them from their analysis options files just to help keep the world a little cleaner. But we can't let them know if they're still in the core, recommended, or flutter sets because then there would be too much noise. Hence the desire to remove them from these sets.

I wouldn't expect to get a lint for an included set of lints - the lint should only show up where they are explicitly included (so the core lint sets themselves). We could add a comment there for the reasoning and call it good?

@bwilkerson
Copy link
Member

I think that's a great idea, and after talking with Phil that's the approach we've decided to take. Thanks!

@pq
Copy link
Member Author

pq commented Jan 30, 2023

💯 Thanks @jakemac53!

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Jan 31, 2023
See: dart-lang/lints#95

Change-Id: Ibc71c71027dc6d7b057e1caf201694394461e01c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280123
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
@mit-mit
Copy link
Member

mit-mit commented Mar 15, 2023

@pq are you working on this one?

@pq
Copy link
Member Author

pq commented Mar 16, 2023

There's a question about dart-lang/linter#3893 to @srawlins. Once we resolve that we can push ahead here I think.

@pq
Copy link
Member Author

pq commented Mar 20, 2023

OK. Revisiting this, my new thinking is that we only want to remove always_require_non_null_named_parameters for a Dart 3.0 world. It only makes sense pre-2.12 and we can no longer opt back that far. As for prefer_equal_for_default_values it's a no-op in 2.19 and beyond but valuable for libraries opted back earlier. The unrelated type lint deprecation/consolidation has been delayed.

CL coming.

@mit-mit
Copy link
Member

mit-mit commented Mar 28, 2023

@pq is this done?

@pq
Copy link
Member Author

pq commented Mar 28, 2023

At this point, we just need to land #107 and publish. Since this bumps the lower SDK bound to 3.0.0 I was imagining we'd wait until a time closer to when such an SDK exists but maybe that's silly?

@mit-mit
Copy link
Member

mit-mit commented Apr 19, 2023

@pq with the last beta done, I think we can go ahead and publish?

https://dart.dev/tools/pub/publishing#publishing-prereleases

@pq
Copy link
Member Author

pq commented Apr 19, 2023

I think so. It sounds like I should land #107 and then publish it as a preview?

@mit-mit
Copy link
Member

mit-mit commented Apr 24, 2023

Commented in #107

@pq
Copy link
Member Author

pq commented Apr 26, 2023

Thanks all. #107 is landed and we're newly published at https://pub.dev/packages/lints/versions/2.1.0.

image

Closing as done.

@pq pq closed this as completed Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants