Skip to content

Fix some broken doc references - #62317

Merged
tvolkert merged 3 commits into
flutter:masterfrom
tvolkert:fix_docs
Jul 28, 2020
Merged

Fix some broken doc references#62317
tvolkert merged 3 commits into
flutter:masterfrom
tvolkert:fix_docs

Conversation

@tvolkert

@tvolkert tvolkert commented Jul 27, 2020

Copy link
Copy Markdown
Contributor

Description

This begins to fix our broken doc references. There's too many breakages to fix outright, so I'm gonna bail on the direct fix and send a PR to add an allowed list for the existing failures, so we can drive that list to zero and keep from making things worse. In the meantime, this PR makes things slightly better.

Related Issues

#31931

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@fluttergithubbot fluttergithubbot added p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Jul 27, 2020
HansMuller
HansMuller approved these changes Jul 27, 2020
View reviewed changes

@HansMuller HansMuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. The changes in slider_theme.dart are substantial, @JoseAlba and/or @clocksmith should have a look at them.

@tvolkert

tvolkert commented Jul 27, 2020

Copy link
Copy Markdown
Contributor Author

The changes in slider_theme.dart are substantial

Yeah, they were declaring dartdoc templates at the top of the file, which doesn't work -- it ends up inlining those templates into the class doc, which isn't what was intended.

@tvolkert

tvolkert commented Jul 27, 2020

Copy link
Copy Markdown
Contributor Author

The test failures are real - with this change, we're (correctly) exporting src/material/curves.dart, but the symbols therein are colliding with the animations package.

@HansMuller

HansMuller commented Jul 27, 2020

Copy link
Copy Markdown
Contributor

@goderbauer - you should take a look at #62317 (comment)

@tvolkert

tvolkert commented Jul 27, 2020

Copy link
Copy Markdown
Contributor Author

The conflict comes from:

@goderbauer goderbauer mentioned this pull request Jul 27, 2020
Merged
@goderbauer

goderbauer commented Jul 27, 2020

Copy link
Copy Markdown
Member

We should just remove the copy of Curves from the animations package and use the definition rom material: flutter/packages#182

@goderbauer

goderbauer commented Jul 27, 2020

Copy link
Copy Markdown
Member

Thanks for doing this clean-up by the way. I started some doc cleanup as well in #62167 and #62071. I was gonna look at some of the broken references next.

BTW, I was wondering if we could clean up the references one folder at a time (e.g. material, widget, etc.) and then place a dartdoc_options.yaml in that folder that elevates the (ignored) warning of a broken doc references to an actual error.

@goderbauer goderbauer mentioned this pull request Jul 27, 2020
Merged
10 of 16 tasks
goderbauer
goderbauer reviewed Jul 28, 2020
View reviewed changes
Comment thread packages/flutter/lib/material.dart Outdated

@goderbauer goderbauer Jul 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we do this one in a separate PR to make a potential revert easier? This one is a little more risky and may break things (see animations package).

@tvolkert tvolkert Jul 28, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ack - will update it tomorrow morning

@tvolkert tvolkert Jul 28, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@tvolkert tvolkert mentioned this pull request Jul 28, 2020
Merged
goderbauer
goderbauer approved these changes Jul 28, 2020
View reviewed changes

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@goderbauer

goderbauer commented Jul 28, 2020

Copy link
Copy Markdown
Member

Looks like this one needs to be rebased ontop the latest master to pick up the version resolvment fix to make cirrus happy.

@tvolkert
tvolkert merged commit ca717c2 into flutter:master Jul 28, 2020
@tvolkert
tvolkert deleted the fix_docs branch July 28, 2020 17:35
tvolkert added a commit that referenced this pull request Jul 28, 2020
@tvolkert
Export Material curves in the Material lib (#62421)
80237ef 
(split out from #62317)
@iamEtornam iamEtornam mentioned this pull request Jul 30, 2020
Merged
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
@tvolkert @Pragya007
Fix some broken doc references (flutter#62317)
3cf9bf4 
flutter#31931
Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020
@tvolkert @Pragya007
Export Material curves in the Material lib (flutter#62421)
cf6e610 
(split out from flutter#62317)
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@tvolkert @mingwandroid
Fix some broken doc references (flutter#62317)
0ef3461 
flutter#31931
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@tvolkert @mingwandroid
Export Material curves in the Material lib (flutter#62421)
c5dc24d 
(split out from flutter#62317)
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@goderbauer goderbauer goderbauer approved these changes
@JoseAlba JoseAlba Awaiting requested review from JoseAlba
+2 more reviewers
@clocksmith clocksmith clocksmith approved these changes
@HansMuller HansMuller HansMuller approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

framework flutter/packages/flutter repository. See also f: labels. p: cupertino_ui cupertino_ui package in flutter/packages p: material_ui material_ui package in flutter/packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tvolkert @HansMuller @goderbauer @clocksmith @googlebot @fluttergithubbot