Skip to content

Enable strong mode in Dartdoc #1611

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 6 commits into from
Feb 26, 2018
Merged

Enable strong mode in Dartdoc #1611

merged 6 commits into from
Feb 26, 2018

Conversation

jcollins-g
Copy link
Contributor

Fixes #1561, for great justice. The strong mode change shifts a lot of types from dynamic to something inferred. The accompanying refactor also reveals some inherited operators that were disappeared in previous versions (MultiplyInheritedExecutableElements were silently dropped on the floor, as were operators inherited from private classes).

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Feb 23, 2018
@kevmoo
Copy link
Member

kevmoo commented Feb 24, 2018

QQ: I just tried it out on the SDK docs.

Noticed that (for example)

Before: NumberList from dart:svg - operator + linked to List operator+ – expected
With this update: it links to dart:collection ListMixin operator+ – which seems weird

@kevmoo
Copy link
Member

kevmoo commented Feb 24, 2018

...is this the difference between linking to where the "doc comment" is vs where we're getting the implementation?

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.

Seems like a clear improvement, even taking into account the issue Kevin found.

@@ -5091,6 +5030,7 @@ class PackageBuilder {
PerformanceLog log = new PerformanceLog(null);
AnalysisDriverScheduler scheduler = new AnalysisDriverScheduler(log);
AnalysisOptionsImpl options = new AnalysisOptionsImpl();
options.strongMode = true;
Copy link
Member

Choose a reason for hiding this comment

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

👍

pubspec.lock Outdated
@@ -415,4 +415,4 @@ packages:
source: hosted
version: "2.1.13"
sdks:
dart: ">=2.0.0-dev <=2.0.0-dev.24.0"
dart: ">=2.0.0-dev <=2.0.0-edge.0d5cf900b021bf5c9fa593ffa12b15bcd1cc5fe0"
Copy link
Member

Choose a reason for hiding this comment

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

You'll likely want something like 2.0.0-dev.30.0 instead of a local build number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kevmoo
Copy link
Member

kevmoo commented Feb 25, 2018

Seems like a clear improvement, even taking into account the issue Kevin found.

Not even sure it's an issue, @devoncarew

@jcollins-g
Copy link
Contributor Author

jcollins-g commented Feb 26, 2018

@kevmoo Yours is an example where prior to this PR dartdoc was getting it wrong because it prioritized interfaces over classes here: https://github.com/dart-lang/dartdoc/pull/1611/files#diff-4d22582de7e25fdddcfff99652835a1eL791. But because it did so without emitting a warning and noone filed a bug about it, it was off my radar.

I can only speculate how this came to be originally as the prioritization of interfaces happened long before I started working on dartdoc. I'd guess it is related to the issue described in #1561 -- unless it prioritized interfaces there were cases where we'd lose information about multiply inherited elements.

Regardless, with the analyzer doing the right thing with strong mode enabled we no longer have to decide between a couple slightly wrong ways of resolving this and can simply do it right. The implementation of that operator comes from ListMixin, and so the new docs have correct linkage.

@jcollins-g jcollins-g merged commit 8ff3d14 into master Feb 26, 2018
@jcollins-g jcollins-g deleted the strong-mode-forever branch February 26, 2018 20:19
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