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

flutter docs are missing lots of classes #1236

Closed
devoncarew opened this issue Aug 29, 2016 · 18 comments
Closed

flutter docs are missing lots of classes #1236

devoncarew opened this issue Aug 29, 2016 · 18 comments
Labels
customer-flutter Issues originating from important to Flutter P0 A serious issue requiring immediate resolution

Comments

@devoncarew
Copy link
Member

The 0.9.7+2 release introduced an issue where the generated docs for flutter are missing many classes.

This page: https://docs.flutter.io/flutter/material/material-library.html

only shows ~5 classes.

@devoncarew devoncarew added P1 A high priority bug; for example, a single project is unusable or has many test failures customer-flutter Issues originating from important to Flutter labels Aug 29, 2016
@devoncarew
Copy link
Member Author

flutter/flutter#5644

@sethladd
Copy link
Contributor

Is this still the case?

@Hixie Hixie added P0 A serious issue requiring immediate resolution and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Oct 18, 2016
@Hixie
Copy link
Contributor

Hixie commented Oct 18, 2016

Moving to P0 because if this isn't fixed then it's going to block deployment of subsequent fixes.

@eseidelGoogle
Copy link
Contributor

Correct, we're intentionally pinned on an older version of dartdoc due to this regression:
https://github.com/flutter/flutter/blob/master/dev/bots/docs.sh#L5

@astashov
Copy link
Contributor

astashov commented Dec 13, 2016

It seems like the bug is in analyzer. It got broken at this commit, when the analyzer version was bumped to ^0.28.1 (and it's still broken with 0.29.1). If I just change the analyzer version back to 0.27 at that commit and nothing else, it works just fine and generates all the classes successfully.

It seems like that happens because by some reason analyzer doesn't gather exported namespaces correctly. E.g., there:

_exportedNamespace =
, analyzer 0.27 returns the namespace instance with like 600+ records inside, and analyzer 0.28 returns the namespace with just ~10 records.

@devoncarew do you want me to dig it deeper, or you will pass this to the analyzer team?

@keertip
Copy link
Collaborator

keertip commented Dec 13, 2016

@bwilkerson , should dartdoc be doing something else instead of

_exportedNamespace =
        new NamespaceBuilder().createExportNamespaceForLibrary(element);

to get all the public elements for a library? Has this api changed?

@bwilkerson
Copy link
Member

No, the API hasn't changed, and the code hasn't changed since 6/16, and most of it has been stable since 4/16. I don't see any obvious cause for the issue. I'll need to get a reproduction case so that I can step through the creation of the namespace.

@keertip
Copy link
Collaborator

keertip commented Dec 13, 2016

@astashov , how did you run dartdoc on flutter?

@eseidelGoogle
Copy link
Contributor

eseidelGoogle commented Dec 13, 2016

https://github.com/flutter/flutter/blob/master/dev/bots/docs.sh will generate all of Flutter's docs. You'll note that it intentionally pins us to an old version of dartdoc to avoid this bug.

Travis does our doc generation on every commit. https://github.com/flutter/flutter/blob/master/.travis.yml is its config. Just two scripts it runs. :)

@astashov
Copy link
Contributor

To use a debugger, I run it somewhat like this:

git clone https://github.com/flutter/flutter.git ~/projects/flutter
cd ~/projects/flutter
bin/flutter update-packages
git clone https://github.com/dart-lang/dartdoc.git ~/projects/dartdoc
cd ~/projects/dartdoc
pub get

DART_SDK=~/projects/flutter/bin/cache/dart-sdk FLUTTER_ROOT=~/projects/flutter bin/dartdoc.dart \
  --header ~/projects/flutter/dev/docs/styles.html \
  --header ~/projects/flutter/dev/docs/analytics.html \
  --exclude temp_doc \
  --favicon=~/projects/flutter/dev/docs/favicon.ico \
  --use-categories \
  --include-external animation \
  --include-external cassowary \
  --include-external foundation  \
  --include-external gestures \
  --include-external http \
  --include-external material \
  --include-external painting \
  --include-external physics \
  --include-external rendering \
  --include-external scheduler \
  --include-external services \
  --include-external widgets \
  --include-external driver_extension \
  --include-external flutter_driver \
  --include-external flutter_markdown \
  --include-external flutter_markdown_raw \
  --include-external flutter_test \
  --input ~/projects/flutter/dev/docs \
  --output ~/projects/flutter/dev/docs/doc/api

I can try to provide SSCCE for it, if you want.

@keertip
Copy link
Collaborator

keertip commented Dec 14, 2016

Found the problem - dartdoc is picking up the wrong library named material. It is generating docs for flutter/lib/src/material/material.dart instead of flutter/lib/material.dart. Working on making the --include-external flag more strict. Once that is in place, can tweak the flutter docs script to pass in more unambiguous names for the libraries.

@eseidelGoogle
Copy link
Contributor

Fantastic! Thank you both!

@astashov
Copy link
Contributor

@keertip woohoo, great job! I'll move on to #1153 then :)

@eseidelGoogle
Copy link
Contributor

Should library name collisions have caused (or cause in the future) some sort of warning inside dartdoc or the analyzer? We're happy to update names and/or be more specific for flags like --include-external as necessary in Flutter.

@sethladd
Copy link
Contributor

sethladd commented Dec 14, 2016 via email

@sethladd
Copy link
Contributor

Thanks @keertip ! Do we need to do anything on the Flutter side?

eseidelGoogle added a commit to eseidelGoogle/dartdoc that referenced this issue Dec 14, 2016
@eseidelGoogle
Copy link
Contributor

Now that @keertip and @astashov did the hard work of figuring out what was wrong. :) I decided to slap together an alternative fix which includes the warnings I had imagined:
https://github.com/eseidelGoogle/dartdoc/tree/external_warnings

If that's useful to either @devoncarew or @keertip, please feel free to take it! (Note that in that commit it's using the old match function instead of @keertip's new one, the new one is included commented out.)

Note that running with that patch I immediately see more potential problems with the current system (even after the fix):

ERROR: animation matches multiple libraries: (/src/flutter/packages/flutter/lib/animation.dart, /src/flutter/packages/flutter/lib/src/animation/animation.dart)
ERROR: http matches multiple libraries: (/src/flutter/packages/flutter/lib/src/http/http.dart, /src/flutter/packages/flutter/lib/http.dart)
ERROR: material matches multiple libraries: (/src/flutter/packages/flutter/lib/src/material/material.dart, /src/flutter/packages/flutter/lib/material.dart)
WARNING: driver_extension matches no libraries.

@devoncarew
Copy link
Member Author

Thanks Eric! The additional checks make sense; better to fast-fail here if there's ambiguity about the library to gen docs for.

abarth added a commit to flutter/flutter that referenced this issue Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-flutter Issues originating from important to Flutter P0 A serious issue requiring immediate resolution
Projects
None yet
Development

No branches or pull requests

7 participants