Skip to content

Handle reexports of extensions in dartdoc #2045

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 2 commits into from
Oct 23, 2019
Merged

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Oct 16, 2019

Fixes #2032.

This implements canonicalization with export handling for extensions, but not yet their methods. (All methods of extensions are assumed canonical with their extension, which is usually the case.) Largely based on the class handling for the same problem, which has also been cleaned up a bit.

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Oct 16, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 93.938% when pulling fd0b445 on export-import-handling into f6d7e4f on master.

}
for (LibraryElement le in _libraryElement.exportedLibraries) {
extensionElements.addAll(le.definingCompilationUnit.extensions
.where((t) => _exportedNamespace.definedNames.values.contains(t.name)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried this might not do the right thing. Consider files foo.dart and bar.dart, each of which defines a distinct extension called E. Then if we have a library like this:

export 'foo.dart' show E;
export 'bar.dart' hide E;

Then I think both Es will get included in extensionElements, because both of them have a name that matches a name that exists in _exportedNamespace.

The best correct approach I can currently think of is:

  • Expose an analyzer API to allow clients to figure out which elements are actually exported by an ExportElement (e.g. we could expose dartdoc to filter a set of elements via namespace combinators (we could move NamespaceBuilder._applyCombinators to ExportElement and re-name it appropriately)
  • Change this code to iterate through the export elements rather than the exported libraries, so that it can use the newly exposed API to figure out exactly which extensions are shown.

It's ok with me if you want to file an issue and address this in a later PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm seeing this now that I'm looking more closely at the library scope code in the analyzer. It looks like this has always been wrong in the class implementation. The "LibraryExportScope" I mentioned in chat the other day is probably the right solution for this, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix for this is taking a while, I think I will go ahead and deal with this in a followup PR.

@jcollins-g jcollins-g merged commit 882d0b5 into master Oct 23, 2019
@jcollins-g jcollins-g deleted the export-import-handling branch April 27, 2020 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exported extensions don't show up in the docs for a library
4 participants