Skip to content

Conversation

jcollins-g
Copy link
Contributor

Make @canonicalFor no longer able to be detected from macros or tools, but in so doing restrict tool executions to only documentation that will be used (dartdoc-canonical ModelElement instances, or ModelElements that have no canonicalization).

As an aside, this seems to improve dartdoc's overall performance on flutter by about 15%.

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Jan 11, 2019
@jcollins-g jcollins-g requested review from gspencergoog and pq January 11, 2019 01:45
if (_libraryDocs == null) {
_libraryDocs = _setCanonicalFor(super.documentation);
// TODO(jcollins-g): warn if a macro/tool _does_ generate an unexpected
// canonicalFor?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that's a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't seem super urgent so I didn't bother here, but in the long run...

@coveralls
Copy link

coveralls commented Jan 11, 2019

Coverage Status

Coverage increased (+0.02%) to 93.808% when pulling 154e1bd on too-many-tools into 10f30e8 on master.

Copy link
Member

@pq pq left a comment

Choose a reason for hiding this comment

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

I may well be missing nuances but on the surface this looks like exactly what we want. Thanks!

jcollins-g added a commit that referenced this pull request Jan 11, 2019
* canonicalFor must be declared literally in the library

* Add tests for minimizing tool execution

* dartfmt

* Workaround problem with mixins on 2.1.0 stable

* Review comments
@jcollins-g jcollins-g merged commit a6b4e7d into master Jan 11, 2019
@jcollins-g jcollins-g deleted the too-many-tools branch January 11, 2019 20:22
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.

5 participants