[M] Change in dart/sdk[main]: analyzer: Tidy up documentation for the new Deprecated annotation war...
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 4 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
4 comments
This annotation indicates that extending the annotated class is deprecated
and will soon be removed. This change will likely be enforced by markingThis doesn't read well IMO. It seems wrong to say that "extending the class will be removed". I liked "ability" (or "capability"); it sounded more correct. How about "the ability to extend the annotated class is deprecated"? (Here and below.)
DEPRECATED_SUBCLASS:I hadn't thought about it before, but 'subclass' seems wrong. I think we use 'subclass' for 'extends' relationships, so it shouldn't include 'implements'. We use 'subtype' for 'extends', 'implements', and 'with', so it also doesn't really fit this case. I'm not sure why we have a way to only disabling 'extends' and 'implements' relationships, but there isn't a really good name for this.
And maybe it's too late to do anything about it, but I think it's worth fixing if we can.
This annotation indicates that extending or implementing the annotatedI think this emphasizes the terminology problem. Ideally we'd write "the ability to subclass" or "the ability to subtype", but neither of those would imply the correct semantics.
If no instructions are present, then remove the relevant `extends` clauseThis is interesting. In the other docs we're saying "Otherwise", implying "if you don't follow the instructions", but this is a fairly different statement "only do this if there are no instructions". I think "Otherwise" is better.
Related details
- Connie Ooi
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 3 comments![]( "Open in Gerrit")
This annotation indicates that extending the annotated class is deprecated
and will soon be removed. This change will likely be enforced by markingThis doesn't read well IMO. It seems wrong to say that "extending the class will be removed". I liked "ability" (or "capability"); it sounded more correct. How about "the ability to extend the annotated class is deprecated"? (Here and below.)
I like it. Done.
DEPRECATED_SUBCLASS:I hadn't thought about it before, but 'subclass' seems wrong. I think we use 'subclass' for 'extends' relationships, so it shouldn't include 'implements'. We use 'subtype' for 'extends', 'implements', and 'with', so it also doesn't really fit this case. I'm not sure why we have a way to only disabling 'extends' and 'implements' relationships, but there isn't a really good name for this.
And maybe it's too late to do anything about it, but I think it's worth fixing if we can.
I think we use 'subclass' for 'extends' relationships, so it shouldn't include 'implements'.
That's not how I think of 'subclass'. @l...@google.com proposed this name. I think it's fine, but we have time to change it, if we should.
If no instructions are present, then remove the relevant `extends` clauseThis is interesting. In the other docs we're saying "Otherwise", implying "if you don't follow the instructions", but this is a fairly different statement "only do this if there are no instructions". I think "Otherwise" is better.
- Brian Wilkerson
- Connie Ooi
- Lasse Nielsen
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Brian Wilkerson
- Connie Ooi
- Lasse Nielsen
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: pkg/analyzer/messages.yaml
Insertions: 8, Deletions: 10.
The diff is too large to show. Please review the diff.
```
Change information
analyzer: Tidy up documentation for the new Deprecated annotation
warnings.
- M pkg/analyzer/messages.yaml
Code-Review: +1 by Brian Wilkerson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen added 3 comments![]( "Open in Gerrit")
DEPRECATED_SUBCLASS:Samuel RawlinsI hadn't thought about it before, but 'subclass' seems wrong. I think we use 'subclass' for 'extends' relationships, so it shouldn't include 'implements'. We use 'subtype' for 'extends', 'implements', and 'with', so it also doesn't really fit this case. I'm not sure why we have a way to only disabling 'extends' and 'implements' relationships, but there isn't a really good name for this.
And maybe it's too late to do anything about it, but I think it's worth fixing if we can.
I think we use 'subclass' for 'extends' relationships, so it shouldn't include 'implements'.
That's not how I think of 'subclass'. @l...@google.com proposed this name. I think it's fine, but we have time to change it, if we should.
If "subclass of A" means something specific, other than just "being a class and being a subtype of A", it probably means extending, directly or transitively, the class `A`. I would use it as the former, but there are people using it as the latter, which is the problem here.
So I don't think "subclass" is used consistently. I try to avoid using it because it might be misunderstood. Sometimes it's used for "extends" relations, sometimes it's just used for being a class *and* a subtype. Sometimes it's used for one level of subclassing, sometimes it's used recursively (and then distinguished by "immediate subclass" or "transitive subclass").
Or when being really pedantic, I use "declared subclass" (the class syntactically referenced by the `extends` clause of a class) for the specific class (whether there are mixin applications between it and this class).
I don't have a better name for this annotation, "subclass" is the word *I* would use, and I don't have a better word for "create a class that directly extends or implements".
This annotation indicates that extending or implementing the annotatedLasse NielsenI think this emphasizes the terminology problem. Ideally we'd write "the ability to subclass" or "the ability to subtype", but neither of those would imply the correct semantics.
Ack. Or more directly
```
Annotation on a class which will remove the ability to
extend or implement that class, and if the class is
a mixin class, the ability to mix in that class.
The change could be the class becoming `final` or `sealed`.
```
I include mixing in a mixin-class because that is also "subclassing" to me,
so it feels like it should be included if using that name. Most classes are not mixin classes, and it won't affect anything for those. If you do have a mixin class that you want to remove the ability to extend and implement, but not the ability to mix in, then you can still use `.implement` and `.extend`. That probably means it becomes a `mixin` declaration, so you may also remove the ability to instantiate, and one deprecation won't be enough anyway.
("Indicates" is a no-content word. Yoda was right: Do or do not! There is no "indicates".)
annotation. Otherwise, remove the relevant `extends` clause or remove the"annotation," -> "annotation's deprecation message,",
or just "annotation's message,".
(Maybe, or maybe I'm being too nit-picky. If the reader will understand this, then it's fine.)
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |