-
Notifications
You must be signed in to change notification settings - Fork 224
Some more minor dartdoc typos #606
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
Conversation
|
|
||
| import 'src/backend/declarer.dart'; | ||
| import 'src/backend/invoker.dart'; | ||
| import 'src/backend/platform_selector.dart' show PlatformSelector; // for the dartdocs |
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.
Won't this cause an unused import error?
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.
Apparently it only caused a dartfmt error.
In any case, I'm not sure this is the right fix; @jcollins-g may have better ideas. I just happened to notice that we're referring to [PlatformSelector] but dartdocs doesn't know why or what to link to.
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.
If it does cause an "unused import" warning, that should be fixed. Use in DartDoc should count as use.
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.
@lrhn Now that dartdoc autoimports everything under the sun, that may not be worth it anymore. For Flutter, at least, we wouldn't want to have imports that are only used by dartdocs.
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.
I don't know how DartDoc has been changed, but references in DartDoc should be resolved the same way identifiers outside of the DartDoc is. That means that the only way to ensure that the DartDoc points to the thing you want is to import that thing and refer to it by name. Anything other than that is too magic to depend on.
(That is, I didn't ask for DartDoc to change, and I actually didn't want it to, I just wanted the "unused warning" to go away in the analyzer).
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.
Dartdoc these days assumes everything it knows about is in scope for every dartdoc reference. That way we can have documentation at low layers (e.g. in dart:ui) that reference higher layers (e.g. package:flutter) which it otherwise couldn't import, either for technical reasons (like in the dart:ui->package:flutter case) or for policy reasons (e.g. we don't want it to be possible for code from the widgets layer to be in scope at the rendering layer, since that would make it too easy to introduce bidirectional dependencies in the implementation).
|
If the link is being referenced from something that isn't part of the public interface, the warning is a dartdoc bug. Assuming that's not the case though, PlatformSelector will make no sense to dartdoc unless it is part of the public interface of the package (in which case we will link to it), or if it is in the namespace where it is referenced (@Hixie's fix). An alternative that should work to get rid of the warning is to specify the library name (even if that is private), I'm not sure how to do that with a default-named library though. It will be something like 'libname.PlatformSelector'. That is probably a better choice than introducing an "unused" import. |
|
I spoke with @munificent and this is not a pattern we want to support until we have a better solution. We shouldn't be importing packages just for the sake of dart docs. I'm told there were some discussions of having specialized imports just for this case though. |
|
I think typically we replace |
|
I suggest that if you want dartdoc to support ignoring(*) this kind of warning, go ahead and leave the link in place (and/or file a bug), otherwise @natebosch 's suggestion works well. Either way, dartdoc won't link to it, of course, since PlatformSelector isn't part of the public interface. (*) -- What dartdoc should do with a link to something real that isn't public is warn differently: instead of unresolved doc reference, warn "no canonical library found". That way we can be alerted that we're referencing a class that isn't part of the public interface to the package. |
|
cc @kevmoo who has also seen this issue before. Is there something filed where we can track progress on dart-doc-only imports or similar workarounds? |
|
Closing in favour of #659 which is presumably less controversial. |
I added the import because otherwise the mention of
[PlatformSelector]in the docs has no way to figure out what it's referring to since that class isn't exported.