dart-lang
When publishing a breaking change version, give me warnings about public API marked @deprecated
When publishing a new breaking version (v1.x.y -> v2.0.0) – this is the time to remove cruft.
The pub client can checked to see if the version I'm publishing is breaking. It'd be a great time to me know about any existing @deprecated (or @Deprecated(...)) in the lib dir.
We are considering this might work better as a lint. Then you get the warning before attempting to publish, and it can be ignored with // ignore: lines.
@pq does that make sense to you? Where should the lint be reported? at the deprecation annotation or at the version number in the pubspec.
I don't think we have any other lints that take the version number into account. Do we need to know about the previous version, or is it enough that the current version is a x.0.0, where x>=2 ?
I think I like the idea. I'd like to specify it thoroughly before landing it though. (Once a lint rule is in the wild, it is very hard to add to the reported cases (fixing "false negative" bugs.)
So, for each @Deprecated annotation, annotating an element:
- Don't report if we're not in a pub package with a version.
- Don't report if the annotated element is a private element (not a private field, class, unnamed extension, element in an unnamed extension, static element in a private class, etc. etc.). Public instance members of private classes can be considered public, as they may override elements of a public class.
- However, maybe don't report if the annotated element is an override. If we're overriding an element in an outside class, maybe declared in another package, then maybe we want to keep annotating the override.
- Don't report if the annotated element is not in the public API. We can't really calculate this, in the analyzer, yet. It requires scanning all public files in
libfor exports. And it requires invalidating the information any time a change is made to such exports. For now, I guess we can just use/lib? If the thing-being-annotated is anywhere in/lib, including/lib/src, then call it public API. - Don't report if the pub package version does not represent a "breaking change" release. What counts? 1.0.0 is breaking. So is 2.0.0, 0.1.0, 0.2.0. What about 0.0.1? What about 1.0.0-dev? What about 1.0.0-pre or 1.0.0-beta? If we report this lint at 1.0.0-dev, then the lint might be very noisy, not giving folks a chance to fix things before you light up their Problems panel. If you only report 1.0.0 and not 1.0.0-dev, then there's less chance of over-reporting, but maybe you catch devs off-guard when someone goes to make a quick release PR. I think I favor the latter choice.
I think I like the idea. I'd like to specify it thoroughly before landing it though. (Once a lint rule is in the wild, it is very hard to add to the reported cases (fixing "false negative" bugs.)
Good idea to spend the time getting the details right! Thanks for the thoughtful walkthrough!
So, for each
@Deprecatedannotation, annotating an element:
- Don't report if we're not in a pub package with a version.
Agreed!
- Don't report if the annotated element is a private element (not a private field, class, unnamed extension, element in an unnamed extension, static element in a private class, etc. etc.). Public instance members of private classes can be considered public, as they may override elements of a public class.
What is the meaning of a @deprecated on a non public element? And who is it for? Is that for internal use only - communicating between team members? Or should we have another lint warning against annotating non-public elements?
If it has a meaning, then I guess you are right, the version number is about the public interface, so the lint should only talk about that.
Does the analyzer have a concept of "public element" we can piggy-back on here instead of specifying every detail of what public means?
- However, maybe don't report if the annotated element is an override. If we're overriding an element in an outside class, maybe declared in another package, then maybe we want to keep annotating the override.
Uh - good point. It might be hard to take action on an overriding element. Here comes my biggest issue with this lint: a deprecation usually means "this will go away in the next breaking version" but it can also just be used to mean "don't use this without thinking twice, though we'll never remove this element for backwards compatibility.
My thought is that if you use it in the second way, you should not enable this lint in your code-base or be liberal with // ignore: comments.
We ought to have an annotation saying "warn about uses of this" without the removal commitment... (https://pub.dev/documentation/meta/latest/meta/doNotSubmit-constant.html is very close, and could perhaps be used for this)
- Don't report if the annotated element is not in the public API. We can't really calculate this, in the analyzer, yet. It requires scanning all public files in
libfor exports. And it requires invalidating the information any time a change is made to such exports. For now, I guess we can just use/lib? If the thing-being-annotated is anywhere in/lib, including/lib/src, then call it public API.- Don't report if the pub package version does not represent a "breaking change" release. What counts? 1.0.0 is breaking. So is 2.0.0, 0.1.0, 0.2.0. What about 0.0.1? What about 1.0.0-dev? What about 1.0.0-pre or 1.0.0-beta? If we report this lint at 1.0.0-dev, then the lint might be very noisy, not giving folks a chance to fix things before you light up their Problems panel. If you only report 1.0.0 and not 1.0.0-dev, then there's less chance of over-reporting, but maybe you catch devs off-guard when someone goes to make a quick release PR. I think I favor the latter choice.
I agree, that 1.0.0-dev release should trigger the warning.
In my understanding of semver, it is supposed to be a preview of the api shape 1.0.0 is going to have (without final promises) so it makes sense to have the warning early. I don't think that the "overwhelming" argument is very strong - the alternative is just to overwhelm later.