Skip to content

Commit d59800f

Browse files
authored
Make precaching work throughout the documentationFrom tree (#1934)
1 parent 9e5a57d commit d59800f

File tree

3 files changed

+78
-20
lines changed

3 files changed

+78
-20
lines changed

lib/src/model.dart

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1929,7 +1929,7 @@ abstract class GetterSetterCombo implements ModelElement {
19291929
_documentationFrom.addAll(setter.documentationFrom);
19301930
}
19311931
if (_documentationFrom.length == 0 ||
1932-
_documentationFrom.every((e) => e.documentation == ''))
1932+
_documentationFrom.every((e) => e.documentationComment == ''))
19331933
_documentationFrom = computeDocumentationFrom;
19341934
}
19351935
return _documentationFrom;
@@ -4697,16 +4697,26 @@ class PackageGraph {
46974697

46984698
/// Generate a list of futures for any docs that actually require precaching.
46994699
Iterable<Future> precacheLocalDocs() sync* {
4700+
// Prevent reentrancy.
4701+
Set<ModelElement> precachedElements = new Set();
4702+
4703+
Iterable<Future> precacheOneElement(ModelElement m) sync* {
4704+
for (ModelElement d in m.documentationFrom.where((d) => d.documentationComment != null)) {
4705+
if (needsPrecacheRegExp.hasMatch(d.documentationComment)
4706+
&& !precachedElements.contains(d)) {
4707+
precachedElements.add(d);
4708+
yield d._precacheLocalDocs();
4709+
}
4710+
}
4711+
}
4712+
47004713
for (ModelElement m in allModelElements) {
47014714
// Skip if there is a canonicalModelElement somewhere else we can run this
47024715
// for. Not the same as allCanonicalModelElements since we need to run
47034716
// for any ModelElement that might not have a canonical ModelElement,
47044717
// too.
47054718
if (m.canonicalModelElement != null && !m.isCanonical) continue;
4706-
if (m.documentationComment != null &&
4707-
needsPrecacheRegExp.hasMatch(m.documentationComment)) {
4708-
yield m._precacheLocalDocs();
4709-
}
4719+
yield* precacheOneElement(m);
47104720
}
47114721
}
47124722

test/model_test.dart

Lines changed: 46 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,14 @@ void main() {
121121
group('Tools', () {
122122
Class toolUser;
123123
Class _NonCanonicalToolUser, CanonicalToolUser, PrivateLibraryToolUser;
124+
Class ImplementingClassForTool, CanonicalPrivateInheritedToolUser;
124125
Method invokeTool;
125126
Method invokeToolNoInput;
126127
Method invokeToolMultipleSections;
127128
Method invokeToolNonCanonical, invokeToolNonCanonicalSubclass;
128129
Method invokeToolPrivateLibrary, invokeToolPrivateLibraryOriginal;
130+
Method invokeToolParentDoc, invokeToolParentDocOriginal;
131+
final RegExp packageInvocationIndexRegexp = new RegExp(r'PACKAGE_INVOCATION_INDEX: (\d+)');
129132

130133
setUpAll(() {
131134
_NonCanonicalToolUser = fakeLibrary.allClasses
@@ -134,6 +137,10 @@ void main() {
134137
.firstWhere((c) => c.name == 'CanonicalToolUser');
135138
PrivateLibraryToolUser = fakeLibrary.allClasses
136139
.firstWhere((c) => c.name == 'PrivateLibraryToolUser');
140+
ImplementingClassForTool = fakeLibrary.allClasses
141+
.firstWhere((c) => c.name == 'ImplementingClassForTool');
142+
CanonicalPrivateInheritedToolUser = fakeLibrary.allClasses
143+
.firstWhere((c) => c.name == 'CanonicalPrivateInheritedToolUser');
137144
toolUser = exLibrary.classes.firstWhere((c) => c.name == 'ToolUser');
138145
invokeTool =
139146
toolUser.allInstanceMethods.firstWhere((m) => m.name == 'invokeTool');
@@ -151,24 +158,48 @@ void main() {
151158
(invokeToolPrivateLibrary.definingEnclosingElement as Class)
152159
.allInstanceMethods
153160
.firstWhere((m) => m.name == 'invokeToolPrivateLibrary');
161+
invokeToolParentDoc = CanonicalPrivateInheritedToolUser.allInstanceMethods
162+
.firstWhere((m) => m.name == 'invokeToolParentDoc');
163+
invokeToolParentDocOriginal = ImplementingClassForTool.allInstanceMethods
164+
.firstWhere((m) => m.name == 'invokeToolParentDoc');
154165
packageGraph.allLocalModelElements.forEach((m) => m.documentation);
155166
});
156167

157-
test('does _not_ invoke a tool multiple times unnecessarily', () {
158-
RegExp packageInvocationIndexRegexp =
159-
new RegExp(r'PACKAGE_INVOCATION_INDEX: (\d+)');
160-
expect(invokeToolNonCanonical.isCanonical, isFalse);
161-
expect(invokeToolNonCanonicalSubclass.isCanonical, isTrue);
162-
expect(
163-
packageInvocationIndexRegexp
164-
.firstMatch(invokeToolNonCanonical.documentation)
165-
.group(1),
166-
equals(packageInvocationIndexRegexp
167-
.firstMatch(invokeToolNonCanonicalSubclass.documentation)
168-
.group(1)));
169-
expect(invokeToolPrivateLibrary.documentation, isNot(contains('{@tool')));
170-
expect(
171-
invokeToolPrivateLibraryOriginal.documentation, contains('{@tool'));
168+
test('invokes tool when inherited documentation is the only means for it to be seen', () {
169+
// Verify setup of the test is correct.
170+
expect(invokeToolParentDoc.isCanonical, isTrue);
171+
expect(invokeToolParentDoc.documentationComment, isNull);
172+
// Error message here might look strange due to toString() on Methods, but if this
173+
// fails that means we don't have the correct invokeToolParentDoc instance.
174+
expect(invokeToolParentDoc.documentationFrom, contains(invokeToolParentDocOriginal));
175+
// Tool should be substituted out here.
176+
expect(invokeToolParentDoc.documentation, isNot(contains('{@tool')));
177+
});
178+
179+
group('does _not_ invoke a tool multiple times unnecessarily', () {
180+
test('non-canonical subclass case', () {
181+
expect(invokeToolNonCanonical.isCanonical, isFalse);
182+
expect(invokeToolNonCanonicalSubclass.isCanonical, isTrue);
183+
expect(
184+
packageInvocationIndexRegexp
185+
.firstMatch(invokeToolNonCanonical.documentation)
186+
.group(1),
187+
equals(packageInvocationIndexRegexp
188+
.firstMatch(invokeToolNonCanonicalSubclass.documentation)
189+
.group(1)));
190+
expect(invokeToolPrivateLibrary.documentation, isNot(contains('{@tool')));
191+
expect(
192+
invokeToolPrivateLibraryOriginal.documentation, contains('{@tool'));
193+
});
194+
195+
test('Documentation borrowed from implementer case', () {
196+
expect(packageInvocationIndexRegexp
197+
.firstMatch(invokeToolParentDoc.documentation)
198+
.group(1),
199+
equals(packageInvocationIndexRegexp
200+
.firstMatch(invokeToolParentDocOriginal.documentation)
201+
.group(1)));
202+
});
172203
});
173204

174205
test('can invoke a tool and pass args and environment', () {

testing/test_package/lib/fake.dart

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1021,3 +1021,20 @@ abstract class _NonCanonicalToolUser {
10211021
}
10221022

10231023
abstract class CanonicalToolUser extends _NonCanonicalToolUser {}
1024+
1025+
abstract class ImplementingClassForTool {
1026+
/// Invokes a tool from inherited documentation via `implemented`
1027+
///
1028+
/// {@tool drill}
1029+
/// This is some drill text right here.
1030+
/// {@end-tool}
1031+
void invokeToolParentDoc();
1032+
}
1033+
1034+
/// The method [invokeToolParentDoc] gets its documentation from an interface class.
1035+
abstract class CanonicalPrivateInheritedToolUser implements ImplementingClassForTool {
1036+
@override
1037+
void invokeToolParentDoc() {
1038+
print('hello, tool world');
1039+
}
1040+
}

0 commit comments

Comments
 (0)