Skip to content

Add chips to class pages for class modifiers #3401

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 27, 2023
Merged

Add chips to class pages for class modifiers #3401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c38eed2
Add chips to class pages
jcollins-g Apr 27, 2023
1493562
rebuild
jcollins-g Apr 27, 2023
abf7af6
fullkind no longer shows abstract, that is intentional
jcollins-g Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/resources/styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ h1 .category {
vertical-align: middle;
}

/* The badge under a declaration for things like "const", "read-only", etc. and for the badges inline like Null safety*/
/* The badge under a declaration for things like "const", "read-only", etc. and for the badges inline like sealed or interface */
/* See https://github.com/dart-lang/dartdoc/blob/main/lib/src/model/feature.dart */
.feature {
display: inline-block;
Expand All @@ -570,6 +570,7 @@ a.feature:hover {

h1 .feature {
vertical-align: middle;
margin: 0 -2px 0 0;
}

.source-link {
Expand Down
13 changes: 13 additions & 0 deletions lib/src/generator/templates.runtime_renderers.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6867,6 +6867,19 @@ class _Renderer_InheritingContainer extends RendererBase<InheritingContainer> {
parent: r);
},
),
'displayedLanguageFeatures': Property(
getValue: (CT_ c) => c.displayedLanguageFeatures,
renderVariable: (CT_ c, Property<CT_> self,
List<String> remainingNames) =>
self.renderSimpleVariable(
c, remainingNames, 'List<LanguageFeature>'),
renderIterable: (CT_ c, RendererBase<CT_> r,
List<MustachioNode> ast, StringSink sink) {
return c.displayedLanguageFeatures.map((e) =>
_render_LanguageFeature(e, ast, r.template, sink,
parent: r));
},
),
'element': Property(
getValue: (CT_ c) => c.element,
renderVariable: (CT_ c, Property<CT_> self,
Expand Down
17 changes: 10 additions & 7 deletions lib/src/model/container_modifiers.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'package:collection/collection.dart' show IterableExtension;
import 'package:dartdoc/src/model/language_feature.dart';
import 'package:dartdoc/src/render/language_feature_renderer.dart';

/// Represents a single modifier applicable to containers.
class ContainerModifier implements Comparable<ContainerModifier> {
Expand All @@ -15,6 +16,7 @@ class ContainerModifier implements Comparable<ContainerModifier> {

/// The display order of this modifier.
final int order;

const ContainerModifier._(
this.name, {
required this.order,
Expand All @@ -41,10 +43,11 @@ class ContainerModifier implements Comparable<ContainerModifier> {
static const ContainerModifier mixin = ContainerModifier._('mixin', order: 4);
}

extension ContainerModifiers on Iterable<ContainerModifier> {
/// Returns a string suitable for prefixing the class name in Dartdoc
/// title bars based on given modifiers.
String modifiersAsFullKindPrefix() => sorted((a, b) => a.compareTo(b))
.where((m) => !m.hideIfPresent.any(contains))
.join(' ');
extension BuildLanguageFeatureSet on Iterable<ContainerModifier> {
/// Transforms [ContainerModifiers] into a series of [LanguageFeature] objects
/// suitable for rendering as chips. Assumes iterable is sorted.
Iterable<LanguageFeature> asLanguageFeatureSet(
LanguageFeatureRenderer languageFeatureRenderer) =>
where((m) => !m.hideIfPresent.any(contains))
.map((m) => LanguageFeature(m.name, languageFeatureRenderer));
}
19 changes: 11 additions & 8 deletions lib/src/model/inheriting_container.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'package:dartdoc/src/element_type.dart';
import 'package:dartdoc/src/model/comment_referable.dart';
import 'package:dartdoc/src/model/container_modifiers.dart';
import 'package:dartdoc/src/model/extension_target.dart';
import 'package:dartdoc/src/model/language_feature.dart';
import 'package:dartdoc/src/model/model.dart';
import 'package:dartdoc/src/model_utils.dart' as model_utils;
import 'package:meta/meta.dart';
Expand Down Expand Up @@ -87,14 +88,21 @@ abstract class InheritingContainer extends Container
/// Class modifiers from the Dart feature specification.
///
/// These apply to or have some meaning for [Class]es and [Mixin]s.
late List<ContainerModifier> containerModifiers = [
late final List<ContainerModifier> containerModifiers = [
if (isAbstract) ContainerModifier.abstract,
if (isSealed) ContainerModifier.sealed,
if (isBase) ContainerModifier.base,
if (isInterface) ContainerModifier.interface,
if (isFinal) ContainerModifier.finalModifier,
if (isMixinClass) ContainerModifier.mixin,
];
]..sort();

@override
late final List<LanguageFeature> displayedLanguageFeatures =
containerModifiers
.asLanguageFeatureSet(
packageGraph.rendererFactory.languageFeatureRenderer)
.toList();

late final List<ModelElement> _allModelElements = [
...super.allModelElements,
Expand Down Expand Up @@ -267,12 +275,7 @@ abstract class InheritingContainer extends Container
@override
Library get enclosingElement => library;

String get fullkind {
if (containerModifiers.isNotEmpty) {
return '${containerModifiers.modifiersAsFullKindPrefix()} $kind';
}
return kind;
}
String get fullkind => kind;

@override
bool get hasModifiers =>
Expand Down
33 changes: 27 additions & 6 deletions lib/src/model/language_feature.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,33 @@

import 'package:dartdoc/src/render/language_feature_renderer.dart';

const Map<String, String> _featureDescriptions = {};

const Map<String, String> _featureUrls = {};

/// 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.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'abstract': 'This type can not be directly constructed.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

'base': 'This type can only be extended (not implemented or mixed in).',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... outside of the library in which it is declared"? ☹️

Maybe it's better to be wrong but concise?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably in this context, better to say "This class can..." here, and in each of the ones below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base can apply to mixins too, but class or mixin isn't too long to say where it applies. Will do.

'interface': 'This type can only be implemented (not extended or mixed in).',
'final': 'This type can neither be extended, implemented, nor mixed in.',
'mixin': 'This type can be used as a class and a mixin.',
};

const Map<String, String> _featureUrls = {
// TODO(jcollins-g): link to dart.dev for all links once documentation is
// available.
'sealed':
'https://github.com/dart-lang/language/blob/main/accepted/future-releases/sealed-types/feature-specification.md#sealed-types',
'abstract': 'https://dart.dev/language/classes#abstract-classes',
'base':
'https://github.com/dart-lang/language/blob/main/accepted/future-releases/class-modifiers/feature-specification.md#class-modifiers',
'interface':
'https://github.com/dart-lang/language/blob/main/accepted/future-releases/class-modifiers/feature-specification.md#class-modifiers',
'final':
'https://github.com/dart-lang/language/blob/main/accepted/future-releases/class-modifiers/feature-specification.md#class-modifiers',
'mixin':
'https://github.com/dart-lang/language/blob/main/accepted/future-releases/class-modifiers/feature-specification.md#class-modifiers',
};

/// An abstraction for a language feature; used to render tags ('chips') to
/// notify the user that the documentation should be specially interpreted.
class LanguageFeature {
/// The description of this language feature.
String? get featureDescription => _featureDescriptions[name];
Expand Down
56 changes: 32 additions & 24 deletions test/class_modifiers_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'package:dartdoc/src/model/model.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';

import 'dartdoc_test_base.dart';
import 'src/utils.dart';

extension on InheritingContainer {
String languageFeatureChips() =>
displayedLanguageFeatures.map((l) => l.name).join(' ');
}

void main() {
defineReflectiveSuite(() {
if (classModifiersAllowed) {
Expand Down Expand Up @@ -63,21 +69,21 @@ base mixin O {}
var Mclass = library.classes.named('M');
var Nmixin = library.mixins.named('N');
var Omixin = library.mixins.named('O');
expect(Aclass.fullkind, equals('class'));
expect(Bclass.fullkind, equals('base class'));
expect(Cclass.fullkind, equals('interface class'));
expect(Dclass.fullkind, equals('final class'));
expect(Eclass.fullkind, equals('sealed class'));
expect(Fclass.fullkind, equals('abstract class'));
expect(Gclass.fullkind, equals('abstract base class'));
expect(Hclass.fullkind, equals('abstract interface class'));
expect(Iclass.fullkind, equals('abstract final class'));
expect(Jclass.fullkind, equals('mixin class'));
expect(Kclass.fullkind, equals('base mixin class'));
expect(Lclass.fullkind, equals('abstract mixin class'));
expect(Mclass.fullkind, equals('abstract base mixin class'));
expect(Nmixin.fullkind, equals('mixin'));
expect(Omixin.fullkind, equals('base mixin'));
expect(Aclass.languageFeatureChips(), equals(''));
expect(Bclass.languageFeatureChips(), equals('base'));
expect(Cclass.languageFeatureChips(), equals('interface'));
expect(Dclass.languageFeatureChips(), equals('final'));
expect(Eclass.languageFeatureChips(), equals('sealed'));
expect(Fclass.languageFeatureChips(), equals('abstract'));
expect(Gclass.languageFeatureChips(), equals('abstract base'));
expect(Hclass.languageFeatureChips(), equals('abstract interface'));
expect(Iclass.languageFeatureChips(), equals('abstract final'));
expect(Jclass.languageFeatureChips(), equals('mixin'));
expect(Kclass.languageFeatureChips(), equals('base mixin'));
expect(Lclass.languageFeatureChips(), equals('abstract mixin'));
expect(Mclass.languageFeatureChips(), equals('abstract base mixin'));
expect(Nmixin.languageFeatureChips(), equals(''));
expect(Omixin.languageFeatureChips(), equals('base'));
}

void test_abstractSealed() async {
Expand All @@ -86,7 +92,8 @@ abstract class A {}
sealed class B extends A {}
''');
var Bclass = library.classes.named('B');
expect(Bclass.fullkind, equals('sealed class')); // *not* sealed abstract
expect(Bclass.languageFeatureChips(),
equals('sealed')); // *not* sealed abstract
}

void test_inferredModifiers() async {
Expand Down Expand Up @@ -116,13 +123,14 @@ base class M extends L {}
var Iclass = library.classes.named('I');
var Lclass = library.classes.named('L');
var Mclass = library.classes.named('M');
expect(Bclass.fullkind, equals('sealed class')); // *not* sealed base
expect(Cclass.fullkind, equals('base class'));
expect(Eclass.fullkind, equals('sealed class'));
expect(Fclass.fullkind, equals('interface class'));
expect(Hclass.fullkind, equals('sealed class'));
expect(Iclass.fullkind, equals('final class'));
expect(Lclass.fullkind, equals('sealed class'));
expect(Mclass.fullkind, equals('base class'));
expect(
Bclass.languageFeatureChips(), equals('sealed')); // *not* sealed base
expect(Cclass.languageFeatureChips(), equals('base'));
expect(Eclass.languageFeatureChips(), equals('sealed'));
expect(Fclass.languageFeatureChips(), equals('interface'));
expect(Hclass.languageFeatureChips(), equals('sealed'));
expect(Iclass.languageFeatureChips(), equals('final'));
expect(Lclass.languageFeatureChips(), equals('sealed'));
expect(Mclass.languageFeatureChips(), equals('base'));
}
}
28 changes: 21 additions & 7 deletions test/container_modifiers_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,41 @@
// BSD-style license that can be found in the LICENSE file.

import 'package:dartdoc/src/model/container_modifiers.dart';
import 'package:dartdoc/src/model/language_feature.dart';
import 'package:dartdoc/src/render/language_feature_renderer.dart';
import 'package:test/test.dart';

class TestChipRenderer extends LanguageFeatureRenderer {
@override
String renderLanguageFeatureLabel(LanguageFeature l) => l.name;
}

extension TestChipsRenderer on Iterable<LanguageFeature> {
String asRenderedString() => map((l) => l.featureLabel).join(' ');
}

void main() {
group('fullKind string tests', () {
test('basic', () {
var l = {
var l = [
ContainerModifier.base,
ContainerModifier.interface,
ContainerModifier.abstract
};
expect(l.modifiersAsFullKindPrefix(), equals('abstract base interface'));
]..sort();
expect(l.asLanguageFeatureSet(TestChipRenderer()).asRenderedString(),
equals('abstract base interface'));
});

test('hide abstract on sealed', () {
var l = {ContainerModifier.abstract, ContainerModifier.sealed};
expect(l.modifiersAsFullKindPrefix(), equals('sealed'));
var l = [ContainerModifier.abstract, ContainerModifier.sealed]..sort();
expect(l.asLanguageFeatureSet(TestChipRenderer()).asRenderedString(),
equals('sealed'));
});

test('empty', () {
var l = <ContainerModifier>{};
expect(l.modifiersAsFullKindPrefix(), equals(''));
var l = <ContainerModifier>[];
expect(l.asLanguageFeatureSet(TestChipRenderer()).asRenderedString(),
equals(''));
});
});
}
4 changes: 0 additions & 4 deletions test/end2end/model_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1991,10 +1991,6 @@ void main() {
expect(interfaces[1].name, 'E');
});

test('class title has abstract keyword', () {
expect(Cat.fullkind, 'abstract class');
});

test('class title has no abstract keyword', () {
expect(Dog.fullkind, 'class');
});
Expand Down