Add chips to class pages for class modifiers #3401
Conversation
| const Map<String, String> _featureDescriptions = { | ||
| 'sealed': 'All direct subtypes must be defined in the same library.', | ||
| 'abstract': 'This type can not be directly constructed.', | ||
| 'base': 'This type can only be extended (not implemented or mixed in).', |
There was a problem hiding this comment.
"... outside of the library in which it is declared"?
Maybe it's better to be wrong but concise?
There was a problem hiding this comment.
I am not sure our language team friends would have phrased it like that, but yes, this shorter version is what I was told was going to be advertised.
| const Map<String, String> _featureDescriptions = { | ||
| 'sealed': 'All direct subtypes must be defined in the same library.', | ||
| 'abstract': 'This type can not be directly constructed.', | ||
| 'base': 'This type can only be extended (not implemented or mixed in).', |
There was a problem hiding this comment.
Probably in this context, better to say "This class can..." here, and in each of the ones below?
There was a problem hiding this comment.
base can apply to mixins too, but class or mixin isn't too long to say where it applies. Will do.
| /// the user that the documentation should be specially interpreted. | ||
| const Map<String, String> _featureDescriptions = { | ||
| 'sealed': 'All direct subtypes must be defined in the same library.', | ||
| 'abstract': 'This type can not be directly constructed.', |
There was a problem hiding this comment.
I don't think "can not be directly constructed" is the most relevant bit here for users. Maybe "This class may have abstract members and cannot be directly constructed"?
| /// An abstraction for a language feature; used to render tags to notify | ||
| /// the user that the documentation should be specially interpreted. | ||
| const Map<String, String> _featureDescriptions = { | ||
| 'sealed': 'All direct subtypes must be defined in the same library.', |
There was a problem hiding this comment.
Maybe "The subtypes of this class will be checked for exhaustiveness in switches"? I'm assuming these need to be brief and self contains, and sealed means quite a bit: it really means "This class is abstract and all direct subtypes must be defined in the same library. Switches over the direct subtypes of this class will be checked for exhaustiveness". But that's probably too much for this? So barring that, I think exhaustiveness is the important bit for users.
There was a problem hiding this comment.
The exhaustiveness bit is the most important part for authors of a class, but for users of a class wanting to know its API surface, the exhaustiveness checking it does internally is actually irrelevant AFAIK? Still, maybe making sure people know how to use sealed properly is so important that it trumps describing what's actually visible on the API surface. I'll put this new wording in for now and see how it lands with people.
There was a problem hiding this comment.
I'm not 100% sure what qualifies as API surface here so I'm not sure quite how to classify this. Most class modifiers primarily affect what another class author can do with a class. That is, final says "other class authors are forbidden from extending or implementing this class". But it doesn't affect anything about how non-class authors use the class (that is, it does not affect the API of instances of this class at all). The sealed modifier affects both class-authors and non class-authors. That is, a class author might want to know that a sealed class can't be extended or implemented (outside of its library). But a non class-author will definitely want to know that "this class is intended to be the type of a closed family (an algebraic datatype), and hence it is likely intended to be switched over, and I will get exhaustiveness checking if I do so". Ideally, we'd surface both, but if we surface only one I believe it should be the latter. My reasoning is that if what you care about is "can't be subclassed or implemented outside of the library" just use final. It's every bit as good. The only reason to use sealed is that you want to provide clients of your API with exhaustiveness checking. And therefore, I'd argue that that is the most important thing to surface.
There was a problem hiding this comment.
TL;DR: This sounds reasonable, I agree. If you care, see below for where I was coming from originally.
An extremely broad definition of the API surface of a class (or any declaration) is : What can I do with this class, using the Dart language, that does not require modifying the class itself? So that includes (indirectly) instances as well as class authors, as well as people modifying the library the class is contained in. It's been my belief that in an ideal universe, API documentation should make it clear what can be done with a thing without having to read its source code.
A more (IMHO, mileage may vary) useful, limited definition that covers most cases where people are looking at documentation of something: What can I do with this class that does not involve modifying the class itself or the library (or even, package) it came from? This includes instances as well as class authors like the above, but excludes the case where (for example) a Flutter application developer might be looking to modify Flutter itself to solve a problem. I figure, most of the time a Flutter application developer is not looking to modify Flutter itself as part of their app development but does want to know the behavior of Flutter classes, methods, and so on.
It was this second definition that led me to think that the other aspect of sealed (that it can not be subclassed or implemented outside of the library) was more important in API documentation. But your point about final makes a lot of sense to me. Since the major differences between sealed and final only make sense within the library itself, we probably should highlight that difference. So I'm now in agreement that your suggestion is best.
Fixes #3392.
This adds chips indicating the new class modifier features (as well as adding 'abstract') to each class page. Class modifiers and
abstractare now removed from the breadcrumbs.The implementation reuses parts of the old "language feature" system that we had used for displaying "Null safety" chips everywhere -- fortunately that infrastructure is still in place so we do not have to alter the templates. We should still rename and perhaps reorganize that system a bit: #3400.
Sample screenshot:
