Skip to content

Commit

Permalink
Improve logic of calculating a "fully qualified name" (#3836)
Browse files Browse the repository at this point in the history 
For improvements that I'm working on in another CL, it made sense to tidy up this code. The `_buildFullyQualifiedName` function was unnecessarily recursive and complex. And then the `_fullyQualifiedNameWithoutLibrary` function (a) has a complex name and (b) also worked _further_ to do undo some work done in `_buildFullyQualifiedName`. This CL tidies these two "fields".

* Combine `fullyQualifiedName` and `_buildFullyQualifiedName` into one getter
  (removing one field).
* Combine `fullyQualifiedNameWithoutLibrary` and
  `_fullyQualifiedNameWithoutLibrary` into one late final field, remove
  recursion, and simplify name to `qualifiedName`. Also mark as
  `@visibleForOverriding`. It should be private but because of the mixin
  hierarchy, it is declared in one file, and defined in another.
* Improve documentation.
  • Loading branch information
@srawlins
srawlins authored Aug 19, 2024
1 parent 271c051 commit 0577ca1
Show file tree
Hide file tree
Showing 11 changed files with 107 additions and 123 deletions.
71 changes: 23 additions & 48 deletions lib/src/generator/templates.runtime_renderers.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3974,30 +3974,6 @@ class _Renderer_DocumentationComment
getters: _invisibleGetters['Documentation']!);
},
),
'fullyQualifiedNameWithoutLibrary': Property(
getValue: (CT_ c) => c.fullyQualifiedNameWithoutLibrary,
renderVariable:
(CT_ c, Property<CT_> self, List<String> remainingNames) {
if (remainingNames.isEmpty) {
return self.getValue(c).toString();
}
var name = remainingNames.first;
var nextProperty =
_Renderer_String.propertyMap().getValue(name);
return nextProperty.renderVariable(
self.getValue(c) as String,
nextProperty,
[...remainingNames.skip(1)]);
},
isNullValue: (CT_ c) =>
c.fullyQualifiedNameWithoutLibrary == null,
renderValue: (CT_ c, RendererBase<CT_> r,
List<MustachioNode> ast, StringSink sink) {
_render_String(c.fullyQualifiedNameWithoutLibrary!, ast,
r.template, sink,
parent: r);
},
),
'hasDocumentationComment': Property(
getValue: (CT_ c) => c.hasDocumentationComment,
renderVariable: (CT_ c, Property<CT_> self,
Expand Down Expand Up @@ -10313,29 +10289,6 @@ class _Renderer_ModelElement extends RendererBase<ModelElement> {
parent: r);
},
),
'fullyQualifiedNameWithoutLibrary': Property(
getValue: (CT_ c) => c.fullyQualifiedNameWithoutLibrary,
renderVariable:
(CT_ c, Property<CT_> self, List<String> remainingNames) {
if (remainingNames.isEmpty) {
return self.getValue(c).toString();
}
var name = remainingNames.first;
var nextProperty =
_Renderer_String.propertyMap().getValue(name);
return nextProperty.renderVariable(
self.getValue(c) as String,
nextProperty,
[...remainingNames.skip(1)]);
},
isNullValue: (CT_ c) => false,
renderValue: (CT_ c, RendererBase<CT_> r,
List<MustachioNode> ast, StringSink sink) {
_render_String(c.fullyQualifiedNameWithoutLibrary, ast,
r.template, sink,
parent: r);
},
),
'hasAnnotations': Property(
getValue: (CT_ c) => c.hasAnnotations,
renderVariable: (CT_ c, Property<CT_> self,
Expand Down Expand Up @@ -10783,6 +10736,28 @@ class _Renderer_ModelElement extends RendererBase<ModelElement> {
parent: r, getters: _invisibleGetters['Context']!);
},
),
'qualifiedName': Property(
getValue: (CT_ c) => c.qualifiedName,
renderVariable:
(CT_ c, Property<CT_> self, List<String> remainingNames) {
if (remainingNames.isEmpty) {
return self.getValue(c).toString();
}
var name = remainingNames.first;
var nextProperty =
_Renderer_String.propertyMap().getValue(name);
return nextProperty.renderVariable(
self.getValue(c) as String,
nextProperty,
[...remainingNames.skip(1)]);
},
isNullValue: (CT_ c) => false,
renderValue: (CT_ c, RendererBase<CT_> r,
List<MustachioNode> ast, StringSink sink) {
_render_String(c.qualifiedName, ast, r.template, sink,
parent: r);
},
),
'sourceCode': Property(
getValue: (CT_ c) => c.sourceCode,
renderVariable:
Expand Down Expand Up @@ -15888,11 +15863,11 @@ const _invisibleGetters = {
'documentationLocal',
'element',
'elementDocumentation',
'fullyQualifiedNameWithoutLibrary',
'hasDocumentationComment',
'hasNodoc',
'needsPrecache',
'pathContext',
'qualifiedName',
'sourceFileName'
},
'Element': {
Expand Down
7 changes: 5 additions & 2 deletions lib/src/model/documentation_comment.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,10 @@ mixin DocumentationComment

String? get sourceFileName;

String? get fullyQualifiedNameWithoutLibrary;
/// The name of this element, qualified with any enclosing element(s), up to
/// but not including an enclosing library.
@visibleForOverriding
String get qualifiedName;

p.Context get pathContext;

Expand Down Expand Up @@ -279,7 +282,7 @@ mixin DocumentationComment
'PACKAGE_PATH': package.packagePath,
'PACKAGE_NAME': package.name,
'LIBRARY_NAME': library?.fullyQualifiedName,
'ELEMENT_NAME': fullyQualifiedNameWithoutLibrary,
'ELEMENT_NAME': qualifiedName,
'INVOCATION_INDEX': invocationIndex.toString(),
'PACKAGE_INVOCATION_INDEX': (package.toolInvocationIndex++).toString(),
};
Expand Down
30 changes: 13 additions & 17 deletions lib/src/model/model_element.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
/// The models used to represent Dart code.
library;

import 'dart:collection';
import 'dart:convert';

import 'package:analyzer/dart/element/element.dart';
Expand Down Expand Up @@ -571,18 +572,21 @@ abstract class ModelElement
/// documented.
String get filePath;

/// Returns the fully qualified name.
///
/// For example: 'libraryName.className.methodName'
@override
late final String fullyQualifiedName = _buildFullyQualifiedName(this, name);

late final String _fullyQualifiedNameWithoutLibrary =
fullyQualifiedName.replaceFirst('${library.fullyQualifiedName}.', '');
String get fullyQualifiedName =>
this is Library ? qualifiedName : '${library.name}.$qualifiedName';

@override
String get fullyQualifiedNameWithoutLibrary =>
_fullyQualifiedNameWithoutLibrary;
late final String qualifiedName = () {
var enclosingElement = this.enclosingElement;

var result = name;
while (enclosingElement != null && enclosingElement is! Library) {
result = '${enclosingElement.name}.$result';
enclosingElement = enclosingElement.enclosingElement;
}
return result;
}();

@override
String get sourceFileName => element.source!.fullName;
Expand Down Expand Up @@ -783,14 +787,6 @@ abstract class ModelElement
@override
String toString() => '$runtimeType $name';

String _buildFullyQualifiedName(ModelElement e, String fullyQualifiedName) {
final enclosingElement = e.enclosingElement;
return enclosingElement == null
? fullyQualifiedName
: _buildFullyQualifiedName(
enclosingElement, '${enclosingElement.name}.$fullyQualifiedName');
}

@internal
@override
CommentReferable get definingCommentReferable {
Expand Down
12 changes: 10 additions & 2 deletions lib/src/model/nameable.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,16 @@ import 'package:dartdoc/src/model/package_graph.dart';
mixin Nameable {
String get name;

/// A qualified name, mostly for use in the web search functionality, and for
/// warnings printed in the terminal; not for display use in rendered HTML.
/// A "fully" qualified name, mostly for use in the web search functionality,
/// and for warnings printed in the terminal; not for display use in rendered
/// HTML.
///
/// "Fully" means the name is qualified through the library. For example, a
/// method named 'baz' in a class named 'Bar' in a library named 'foo' has a
/// fully qualified name of 'foo.Bar.baz'.
///
/// As dartdoc can document multiple packages at once, note that such
/// qualifying names may not be unique across all documented packages.
String get fullyQualifiedName => name;

/// The name to use as text in the rendered documentation.
Expand Down
7 changes: 4 additions & 3 deletions test/constant_values_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import 'src/utils.dart';

void main() {
defineReflectiveSuite(() {
defineReflectiveTests(ConstantValuesWithConstructorTearoffsTest);
defineReflectiveTests(ConstantValuesTest);
if (namedArgumentsAnywhereAllowed) {
defineReflectiveTests(ConstantValuesWithNamedArgumentsAnywhereTest);
}
Expand All @@ -22,16 +22,17 @@ void main() {
// test_reflective_loader migration.

@reflectiveTest
class ConstantValuesWithConstructorTearoffsTest extends DartdocTestBase {
class ConstantValuesTest extends DartdocTestBase {
@override
String get libraryName => 'constructor_tearoffs';
String get libraryName => 'constant_values';

void test_nonGenericFunctionReference() async {
var library = await bootPackageWithLibrary('''
void func() {}
const aFunc = func;
''');
var aFuncConstant = library.constants.named('aFunc');
expect(aFuncConstant.fullyQualifiedName, equals('constant_values.aFunc'));
expect(aFuncConstant.constantValue, equals('func'));
}

Expand Down
19 changes: 19 additions & 0 deletions test/constructors_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ abstract final class C {
''');
var c = library.classes.named('C').constructors.first;
expect(c.name, equals('C'));
// TODO(srawlins): This should be `constructors.C.new`.
expect(c.fullyQualifiedName, 'constructors.C.C');
expect(c.isPublic, isTrue);
expect(c.documentationAsHtml, '<p>Constructor.</p>');
}
Expand All @@ -41,6 +43,8 @@ abstract final class C {
''');
var c = library.classes.named('C').constructors.first;
expect(c.name, equals('C'));
// TODO(srawlins): This should be `constructors.C.new`.
expect(c.fullyQualifiedName, 'constructors.C.C');
expect(c.isPublic, isFalse);
expect(c.documentationAsHtml, '<p>Constructor.</p>');
}
Expand All @@ -54,6 +58,8 @@ abstract interface class C {
''');
var c = library.classes.named('C').constructors.first;
expect(c.name, equals('C'));
// TODO(srawlins): This should be `constructors.C.new`.
expect(c.fullyQualifiedName, 'constructors.C.C');
expect(c.isPublic, isFalse);
expect(c.documentationAsHtml, '<p>Constructor.</p>');
}
Expand All @@ -67,6 +73,7 @@ class C {
''');
var c = library.classes.named('C').constructors.first;
expect(c.name, equals('C._'));
expect(c.fullyQualifiedName, 'constructors.C._');
expect(c.isPublic, isFalse);
expect(c.documentationAsHtml, '<p>Constructor.</p>');
}
Expand Down Expand Up @@ -103,6 +110,7 @@ class C {
''');
var c = library.classes.named('C').constructors.first;
expect(c.name, equals('C.named'));
expect(c.fullyQualifiedName, 'constructors.C.named');
expect(c.isPublic, isTrue);
expect(c.documentationAsHtml, '<p>Constructor.</p>');
}
Expand All @@ -129,6 +137,8 @@ class C {
''');
var c = library.classes.named('C').constructors.first;
expect(c.name, equals('C'));
// TODO(srawlins): This should be `constructors.C.new`.
expect(c.fullyQualifiedName, 'constructors.C.C');
expect(c.isPublic, isTrue);
expect(c.documentationAsHtml, '<p>Constructor.</p>');
}
Expand Down Expand Up @@ -156,6 +166,7 @@ enum E {
''');
var e = library.enums.named('E').constructors.first;
expect(e.name, equals('E.named'));
expect(e.fullyQualifiedName, 'constructors.E.named');
expect(e.isPublic, isFalse);
expect(e.documentationAsHtml, '<p>Constructor.</p>');
}
Expand All @@ -170,6 +181,8 @@ enum E {
''');
var e = library.enums.named('E').constructors.first;
expect(e.name, equals('E'));
// TODO(srawlins): This should be `constructors.E.new`.
expect(e.fullyQualifiedName, 'constructors.E.E');
expect(e.isPublic, isFalse);
expect(e.documentationAsHtml, '<p>Constructor.</p>');
}
Expand All @@ -184,6 +197,7 @@ extension type ET(int it) {
var etNamed =
library.extensionTypes.named('ET').constructors.named('ET.named');
expect(etNamed.name, equals('ET.named'));
expect(etNamed.fullyQualifiedName, 'constructors.ET.named');
expect(etNamed.isPublic, isTrue);
expect(etNamed.documentationAsHtml, '<p>Constructor.</p>');
}
Expand All @@ -195,6 +209,7 @@ extension type ET.named(int it) {}
var etNamed =
library.extensionTypes.named('ET').constructors.named('ET.named');
expect(etNamed.name, equals('ET.named'));
expect(etNamed.fullyQualifiedName, 'constructors.ET.named');
expect(etNamed.isPublic, isTrue);
}

Expand All @@ -204,6 +219,8 @@ extension type ET(int it) {}
''');
var et = library.extensionTypes.named('ET').constructors.named('ET');
expect(et.name, equals('ET'));
// TODO(srawlins): This should be `constructors.ET.new`.
expect(et.fullyQualifiedName, 'constructors.ET.ET');
expect(et.isPublic, isTrue);
}

Expand All @@ -216,6 +233,8 @@ extension type ET.named(int it) {
''');
var etNamed = library.extensionTypes.named('ET').constructors.named('ET');
expect(etNamed.name, equals('ET'));
// TODO(srawlins): This should be `constructors.ET.new`.
expect(etNamed.fullyQualifiedName, 'constructors.ET.ET');
expect(etNamed.isPublic, isTrue);
expect(etNamed.documentationAsHtml, '<p>Constructor.</p>');
}
Expand Down
Loading

0 comments on commit 0577ca1

Please sign in to comment.