Skip to content

Warn "not yet implemented" for comment reference resolution for extension methods #2040

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.

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 4 commits into from
Oct 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 7 additions & 3 deletions lib/src/markdown_processor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,13 @@ MatchingLinkResult _getMatchingLinkElement(
// Try expensive not-scoped lookup.
if (refModelElement == null && element is ModelElement) {
Container preferredClass = _getPreferredClass(element);
refModelElement =
_MarkdownCommentReference(codeRef, element, commentRefs, preferredClass)
.computeReferredElement();
if (preferredClass is Extension) {
element.warn(PackageWarning.notImplemented, message: 'Comment reference resolution inside extension methods is not yet implemented');
} else {
refModelElement =
_MarkdownCommentReference(codeRef, element, commentRefs, preferredClass)
.computeReferredElement();
}
}

// Did not find it anywhere.
Expand Down
3 changes: 3 additions & 0 deletions lib/src/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5271,6 +5271,9 @@ class PackageGraph {
warningMessage =
"private API of ${message} is reexported by libraries in other packages: ";
break;
case PackageWarning.notImplemented:
warningMessage = message;
break;
case PackageWarning.unresolvedDocReference:
warningMessage = "unresolved doc reference [${message}]";
referredFromPrefix = 'in documentation inherited from';
Expand Down
5 changes: 5 additions & 0 deletions lib/src/warnings.dart
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ final Map<PackageWarning, PackageWarningDefinition> packageWarningDefinitions =
PackageWarning.noCanonicalFound,
"no-canonical-found",
"A symbol is part of the public interface for this package, but no library documented with this package documents it so dartdoc can not link to it"),
PackageWarning.notImplemented: PackageWarningDefinition(
PackageWarning.notImplemented,
"not-implemented",
"The code makes use of a feature that is not yet implemented in dartdoc"),
PackageWarning.noLibraryLevelDocs: PackageWarningDefinition(
PackageWarning.noLibraryLevelDocs,
"no-library-level-docs",
Expand Down Expand Up @@ -226,6 +230,7 @@ enum PackageWarning {
ambiguousReexport,
ignoredCanonicalFor,
noCanonicalFound,
notImplemented,
noLibraryLevelDocs,
packageOrderGivesMissingPackageName,
reexportedPrivateApiAcrossPackages,
Expand Down
4 changes: 2 additions & 2 deletions test/dartdoc_integration_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ void main() {
expect(outputLines, contains(matches('^ warning:')));
expect(outputLines.last, matches(r'^found \d+ warnings and \d+ errors'));
expect(outputDir.listSync(), isNotEmpty);
}, timeout: new Timeout.factor(2));
}, timeout: Timeout.factor(2));

test('invalid parameters return non-zero and print a fatal-error',
() async {
Expand Down Expand Up @@ -229,7 +229,7 @@ void main() {

File outFile = File(path.join(tempDir.path, 'index.html'));
expect(outFile.readAsStringSync(), contains('footer text include'));
});
}, timeout: Timeout.factor(2));

test('--footer-text excludes version', () async {
String _testPackagePath =
Expand Down
30 changes: 26 additions & 4 deletions test/model_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1904,7 +1904,7 @@ void main() {
});

test('correctly finds all the classes', () {
expect(classes, hasLength(31));
expect(classes, hasLength(33));
});

test('abstract', () {
Expand Down Expand Up @@ -2114,15 +2114,37 @@ void main() {

group('Extension', () {
Extension ext, fancyList;
Method s;
Class extensionReferencer;
Method doSomeStuff, doStuff, s;
List<Extension> extensions;

setUpAll(() {
ext = exLibrary.extensions.firstWhere((e) => e.name == 'AppleExtension');
extensionReferencer = exLibrary.classes.firstWhere((c) => c.name == 'ExtensionReferencer');
fancyList = exLibrary.extensions.firstWhere((e) => e.name == 'FancyList');
doSomeStuff = exLibrary.classes.firstWhere((c) => c.name == 'ExtensionUser')
.allInstanceMethods.firstWhere((m) => m.name == 'doSomeStuff');
doStuff = exLibrary.extensions.firstWhere((e) => e.name == 'SimpleStringExtension')
.instanceMethods.firstWhere((m) => m.name == 'doStuff');
extensions = exLibrary.publicExtensions.toList();
});

// TODO(jcollins-g): implement feature and update tests
test('documentation links do not crash in base cases', () {
packageGraph.packageWarningCounter.hasWarning(doStuff, PackageWarning.notImplemented,
'Comment reference resolution inside extension methods is not yet implemented');
packageGraph.packageWarningCounter.hasWarning(doSomeStuff, PackageWarning.notImplemented,
'Comment reference resolution inside extension methods is not yet implemented');
expect(doStuff.documentationAsHtml, contains('<code>another</code>'));
expect(doSomeStuff.documentationAsHtml, contains('<code>String.extensionNumber</code>'));
});

test('references from outside an extension refer correctly to the extension', () {
expect(extensionReferencer.documentationAsHtml, contains('<code>_Shhh</code>'));
expect(extensionReferencer.documentationAsHtml, contains('<a href="ex/FancyList.html">FancyList</a>'));
expect(extensionReferencer.documentationAsHtml, contains('<a href="ex/AnExtension/call.html">AnExtension.call</a>'));
});

test('has a fully qualified name', () {
expect(ext.fullyQualifiedName, 'ex.AppleExtension');
});
Expand Down Expand Up @@ -2179,11 +2201,11 @@ void main() {
});

test('correctly finds all the extensions', () {
expect(exLibrary.extensions, hasLength(7));
expect(exLibrary.extensions, hasLength(8));
});

test('correctly finds all the public extensions', () {
expect(extensions, hasLength(5));
expect(extensions, hasLength(6));
});
});

Expand Down
25 changes: 24 additions & 1 deletion testing/test_package/lib/example.dart
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,24 @@ extension AnExtension<Q> on WithGeneric<Q> {
int call(String s) => 0;
}

extension SimpleStringExtension on String {
/// Print this and [another].
/// Refer to [indexOf], from [String].
/// Also refer to [extensionNumber].
void doStuff(String another) {
print(this + another);
}

int get extensionNumber => 3;
}

class ExtensionUser {
/// Refer to [String.extensionNumber], which we use here.
void doSomeStuff(String things) {
print(things.extensionNumber + 1);
}
}

/// Extension on List
extension FancyList<Z> on List<Z> {
int get doubleLength => this.length * 2;
Expand Down Expand Up @@ -654,4 +672,9 @@ extension _Shhh on Object {
// Extension with no name
extension on Object {
void bar() { }
}
}


/// This class has nothing to do with [_Shhh], [FancyList], or [AnExtension.call],
/// but should not crash because we referenced them.
class ExtensionReferencer {}