Skip to content
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

Refactor so that docs are generated through the Package class #1659

Merged
merged 11 commits into from
Apr 6, 2018

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Apr 3, 2018

Another refactor on the long march to #739, #1353, and #1454.

By driving all documentation through the Package class, we can provide for canonical ModelElements that aren't documented locally. This also clears some ground for making dartdoc work in non-package scenarios (since a directory in a monolithic source repository can now be implemented as a special kind of Package).

This change should do almost nothing, and that seems to be true for Flutter, although in checked mode at least dartdoc is twice as fast this way.

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Apr 3, 2018
@jcollins-g
Copy link
Contributor Author

Something is wrong on windows, and some of the new tests seem to time out on travis. Looking.

@jcollins-g
Copy link
Contributor Author

Without checked mode, Flutter documentation speed is a very slight improvement. More time is spent in the analyzer with the refactor, and less in dartdoc itself, so it more or less comes out a wash.

@jcollins-g jcollins-g requested review from devoncarew and keertip April 4, 2018 18:22
Copy link
Member

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

in checked mode at least dartdoc is twice as fast this way

👍

for (var clazz in filterNonDocumented(lib.allClasses)) {
generateClass(_packageGraph, lib, clazz);

for (var constructor in filterNonDocumented(clazz.constructors)) {
Copy link
Member

Choose a reason for hiding this comment

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

I see 7 things here w/ pretty much the same pattern.

Not for this PR, but wonder if this could be factored into a helper function so it's more readable – and less likely to have copy-paste errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep.

This can probably be done with a map of Type to Closure in html_generator_instance and helper methods in each ModelElement class to return documentable subitems. That way you could crush this down into some nested expand calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though as I say that, while that's a pattern I've used in Python I've never actually tried to do that in Dart before.

@jcollins-g jcollins-g merged commit 3098910 into master Apr 6, 2018
@jcollins-g jcollins-g deleted the package-as-primary branch April 6, 2018 19:30
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.

4 participants