Add 'override' to features list #1798
Conversation
|
It may additionally make sense to change it from Should we not show the I'll review now... |
There was a problem hiding this comment.
Changes look good!
It may additionally make sense to change it from override since I don't think this is the same as having an @OverRide annotation; one is user added and the other is programmatically calculated.
It looks like you're handling this - removing any associated @override annotation, and only displaying the calculated override.
I thought about it a bit more, and couldn't come up with better text than override. It might be worth thinking about whether you want to drop inherited when displaying override for a node.
| if (_isOverride == null) { | ||
| // The canonical version of the enclosing element (not canonicalEnclosingElement, | ||
| // as that is the element enclosing the canonical version of this element, | ||
| // two different things. Defaults to the enclosing element. |
There was a problem hiding this comment.
nit: opening paran not closed
|
@kwalrath Any suggestions on language? I'm not a huge fan of "override" here. I agree with Devon that it feels wrong, but I also don't like the alternatives we've thought of so far. |
|
"overridden" makes more sense to me, since it's parallel to "inherited" and doesn't sound like a command (you must override this). |
|
Yeah, but in this case it's the parent element which is overridden. Perhaps strict semantics aren't necessary here though, in favor of brevity. |
|
@kwalrath Hmmm. We're really trying to briefly say "This member is inherited from some other class" or "This member overrides a member in some other class" for overrides. I'm a little concerned about "overridden" because that seems to be common usage for the wrong piece (the part in the other class being, well, overridden). https://en.wikipedia.org/wiki/Method_overriding "Overriding" is yet another possibility, but doesn't seem that much better than "override" to me. Despite some discomfort, override does have the advantage of mirroring the Dart annotation for the same thing. |
There was a problem hiding this comment.
I have tried to deal with the inherited+override possibility via an assert which fires in the event we ever declare something with both inherited and override at the same time. I've run with the assert enabled over all of Flutter with the Dart SDK and all the pub packages it documents and haven't found any cases where we do that.
Fields with split-inheritance are handled by indicating the setter/getter is inherited/overriding.
| if (_isOverride == null) { | ||
| // The canonical version of the enclosing element (not canonicalEnclosingElement, | ||
| // as that is the element enclosing the canonical version of this element, | ||
| // two different things. Defaults to the enclosing element. |
Fixes #981.
Adds the text 'override' to the features list if the class member is a declared override of the parent.
Also handles complex cases involving fields and getter/setter declarations:
