-
Notifications
You must be signed in to change notification settings - Fork 118
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
Resolve non-imported symbols in comments [#1153] #1298
Resolve non-imported symbols in comments [#1153] #1298
Conversation
@Hixie noticed, that sometimes, a library, like the Flutter rendering library, wants to refer to another library, like the Flutter widgets library, in the documentation, but doesn't want to introduce a dependency in the code. Currently, there’s no mechanisms in dartdoc, which allows that. This commit adds that. You can use either a short name (e.g. [icon]) or a fully qualified name (like, [material.Icon.icon]), in the HTML docs, it always will be shown as a short name though (is that a desired behavior?). Dartdoc will go over all the libraries, classes, methods, properties, constants of the package that is being documented, and will try to resolve the symbol. If it’s successful, it will add the link to it, same way as when the symbol is in scope. Some outstanding questions: * What to do in case of ambiguity here? Show a warning, crash with an error message, ignore? * Do we want to show short name in case a fully qualified name is provided? I saw in the comments in the ticket (dart-lang#1153 (comment)) that @Hixie would want that. * Anything else? Testing: Unit tests, of course, also tried to generate Flutter docs with that, and observed that the links for previously non-resolved symbols work.
Reviewing now -
I believe @Hixie mentioned fast-fail. We should definitely at least warn. Dartdoc does generate docs unless there are significant parse errors in the code - it's lenient towards somewhat bad source, so you can always generate docs. The approach the analyzer takes is to allow clients to opt-into failing on warnings (
Sounds like (from @Hixie) yes :) |
Re: |
} | ||
final linkedElement = result.element; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the type here (and below).
return new MatchingLinkResult(result.values.first, result.values.first.name); | ||
} else { | ||
if (_emitWarning) { | ||
print("Abiguous reference '${codeRef}' in '${element.fullyQualifiedName}', " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sp: ambiguous
Also, can you get the file and line number here in order to print them out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like: Ambiguous reference to [foo], lib/my_lib.dart, line 100
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, I see that we're not doing this (locating by file and line number) in the original code, so while this would be great, it's not necessary for this PR.
} | ||
|
||
MatchingLinkResult _findRefElementInLibrary(String codeRef, ModelElement element, List<CommentReference> commentRefs) { | ||
final package = element.library.package; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the type here
MatchingLinkResult _findRefElementInLibrary(String codeRef, ModelElement element, List<CommentReference> commentRefs) { | ||
final package = element.library.package; | ||
final Map<String, ModelElement> result = {}; | ||
package.libraries.forEach((library) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lookup should probably live somewhere more general, like as a method on Package
. That would probably also help with performance issues, as the set of all symbols (and their fully-qualified names) could be calculated once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point!
OK, looks like |
* Add types to declarations * Add filename/line number to the warnings * Move the code for gathering all the model elements of a package to the package's accessor and cache it.
@devoncarew I addessed your feedback, thanks! PTAL |
@@ -1425,6 +1425,26 @@ abstract class ModelElement implements Comparable, Nameable, Documentable { | |||
return (_fullyQualifiedName ??= _buildFullyQualifiedName()); | |||
} | |||
|
|||
String get sourceFileName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this can use the function shorthand (=>
)
return (modelElement.element.computeNode() as AnnotatedNode) | ||
.documentationComment | ||
.references; | ||
if ((modelElement.element.computeNode() as AnnotatedNode).documentationComment != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing this cast twice inside this if statement (modelElement.element.computeNode() as AnnotatedNode
) - perhaps create a new local variable to host the AnnotatedNode?
result[modelElement.fullyQualifiedName] = modelElement; | ||
} | ||
}); | ||
package.allModelElements.forEach((modelElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last comment, I promise :)
If we use a Map here, in the event we have multiple matches, the entry in the map could be overridden. So that would hide the fact that the reference is not unique. Perhaps use a List instead?
And we'll be iterating over a lot of elements - I tend to avoid using a closure for that; a for loop will be more performant.
You might do something thuswise:
List<ModelElement> results = [];
for (ModelElement element in package.allModelElements) {
if (codeRef.contains('.')) {
// It's a fully qualified reference.
if (element.fullyQualifiedName == codeRef) {
results.add(element);
break;
}
} else {
if (element.name == codeRef) {
results.add(element);
}
}
}
@astashov, I think there were one or two issues w/ the lookup code - I added a proposal code snippet; everything else looks good. |
Hmm, it seems like there's one drawback with using
|
Isn't that fine? They can all just point to Widget.key. |
Having said that, I don't think we'd want |
Oh right, that's actually fine. Sorry :)
Umm, now I'm lost :) Could you please elaborate? Why you don't want |
Because it's not in scope. Conceptually, the way I imagine this working is that you import all the libraries. But even if you did that, in code, More practically, if we consider every member of every class to be in scope everywhere, we're going to have conflicts up the wazoo. :-) |
So, you don't want |
That would be my recommendation, yeah. |
Cycling back to this, what @Hixie says makes sense. It sounds like Similarly with |
From now on, if the reference was, for example, `[key]`, we won't match `material.Widget.key`, we will only match `material.key` (i.e. top-level `key` symbol). If you still want to match `Widget.key`, you should use either `[Widget.key]` or `[material.Widget.key]`.
Okay, I've updated the PR. So, from now on, if the reference was, for example, That's how you want it to work, right? |
How do I know what value to use for the library name? Is it (exclusively) the identifiers used with |
Yeah, I think so. E.g. |
SGTM |
lgtm! Landing... |
@Hixie noticed, that sometimes, a library, like the Flutter rendering
library, wants to refer to another library, like the Flutter widgets
library, in the documentation, but doesn't want to introduce a
dependency in the code. Currently, there’s no mechanisms in dartdoc,
which allow that.
This commit adds that. You can use either a short name (e.g. [icon]) or
a fully qualified name (like, [material.Icon.icon]), in the HTML docs,
it always will be shown as a short name though (is that a desired
behavior?). Dartdoc will go over all the libraries, classes, methods,
properties, constants of the package that is being documented, and will
try to resolve the symbol. If it’s successful, it will add the link to
it, same way as when the symbol is in scope.
Some outstanding questions:
error message, ignore?
provided? I saw in the comments in the ticket
(handle dartdoc references to non-imported symbols (was: dartdoc-specific imports) #1153 (comment)) that @Hixie would want that.
Testing: Unit tests, of course, also tried to generate Flutter docs with
that, and observed that the links for previously non-resolved symbols
work.
(fixes #1153)