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

Names of anonymous libraries in 'Flutter' package are not stripped of 'package' prefix #3882

Open
srawlins opened this issue Sep 15, 2024 · 7 comments · Fixed by #3883
Open
Assignees
Labels
P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@srawlins
Copy link
Member

As @goderbauer found earlier, when removing the name portion of a library directive, e.g. s/library widgets;/library; changes the URI of the generated docs for the widgets library.

It turns out this should have worked; there is code in Library.dirName that picks a "dir name" for an anonymous library. That code should strip the package prefix from libraries in the "default package." When generating docs for the Flutter SDK, the Flutter "package" is considered "default," but it's name is capitalized: "Flutter." Because of this, the stripping behavior does not work.

This issue prevents the flutter SDK from adopting the unnecessary_library_names lint rule.

@srawlins srawlins added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) P2 A bug or feature request we're likely to work on labels Sep 15, 2024
@srawlins srawlins self-assigned this Sep 15, 2024
@goderbauer
Copy link
Contributor

goderbauer commented Nov 25, 2024

I just gave this another try in flutter/flutter#159445 with dartdoc 8.3.0 and I am running into the same problem:

Generated URL: package-flutter_driver_flutter_driver/FlutterDriver/FlutterDriver.connectedTo.html
Expected URL: flutter_driver/FlutterDriver/FlutterDriver.connectedTo.html

@goderbauer goderbauer reopened this Nov 25, 2024
@srawlins
Copy link
Member Author

srawlins commented Dec 3, 2024

I cannot reproduce :( Can you double check? I just loaded up 8.3.0 locally, and served the docs with

dart pub global run dhttpd --port 8000 and see the following URL:

Screenshot 2024-12-03 at 7 05 24 AM

Are you referring to a specific link to FluttterDriver.connectTo? I found this URL via searching for 'FlutterDriver.connectTo'.

@goderbauer
Copy link
Contributor

@srawlins Did you remove the library name from packages/flutter_driver/lib/flutter_driver.dart [1] before running dartdoc? With the library name (currently checked in) it does not reproduce, without the library name (code is in flutter/flutter#159445) I can reproduce it locally and also on CI [2] with:

dartdoc version: 8.3.0
flutter version: 3.27.0-1.0.pre.643

Executing: (cd "/b/s/w/ir/x/w/flutter/dev/docs" ; /b/s/w/ir/x/w/flutter/bin/flutter pub global run --enable-asserts dartdoc --output /b/s/w/ir/x/w/flutter/dev/docs/doc/flutter --allow-tools --no-validate-links --link-to-source-excludes /b/s/w/ir/x/w/flutter/bin/cache --link-to-source-root /b/s/w/ir/x/w/flutter --link-to-source-uri-template [https://github.com/flutter/flutter/blob/main/%f%#L%l](https://github.com/flutter/flutter/blob/main/%25f%25#L%25l)% --inject-html --use-base-href --header /b/s/w/ir/x/w/flutter/dev/docs/styles.html --header /b/s/w/ir/x/w/flutter/dev/docs/analytics-header.html --header /b/s/w/ir/x/w/flutter/dev/docs/survey.html --header /b/s/w/ir/x/w/flutter/dev/docs/snippets.html --header /b/s/w/ir/x/w/flutter/dev/docs/opensearch.html --footer /b/s/w/ir/x/w/flutter/dev/docs/analytics-footer.html --footer-text /b/s/w/ir/x/w/flutter/dev/docs/footer.html --allow-warnings-in-packages Flutter,platform_integration,flutter,flutter_localizations,integration_test,flutter_web_plugins,flutter_goldens,flutter_test,flutter_driver --exclude-packages analyzer,args,barback,csslib,flutter_goldens,flutter_goldens_client,front_end,fuchsia_remote_debug_protocol,glob,html,http_multi_server,io,isolate,js,kernel,logging,mime,mockito,node_preamble,plugin,shelf,shelf_packages_handler,shelf_static,shelf_web_socket,utf,watcher,yaml --exclude dart:io/network_policy.dart,package:Flutter/temp_doc.dart,package:http/browser_client.dart,package:intl/intl_browser.dart,package:matcher/mirror_matchers.dart,package:quiver/io.dart,package:quiver/mirrors.dart,package:vm_service_client/vm_service_client.dart,package:web_socket_channel/html.dart --favicon /b/s/w/ir/x/w/flutter/dev/docs/favicon.ico --package-order flutter,Dart,platform_integration,flutter_test,flutter_driver --auto-include-dependencies)
dartdoc:stdout: Documenting Flutter...

<lines omitted for brevity>

Unhandled exception:
Exception: Missing "/b/s/w/ir/x/w/flutter/dev/docs/doc/flutter/flutter_driver/FlutterDriver/FlutterDriver.connectedTo.html", which probably means the documentation failed to build correctly.
#0      DartdocGenerator._sanityCheckDocs (file:///b/s/w/ir/x/w/flutter/dev/tools/create_api_docs.dart:748:9)
dart-lang/lints#1      DartdocGenerator.generateDartdoc (file:///b/s/w/ir/x/w/flutter/dev/tools/create_api_docs.dart:696:5)
<asynchronous suspension>
dart-lang/lints#2      main (file:///b/s/w/ir/x/w/flutter/dev/tools/create_api_docs.dart:150:3)
<asynchronous suspension>

[1] https://github.com/flutter/flutter/pull/159445/files#diff-4175a2d5dc31762727066811624f7d7c4a9127cba08df196fcb184bb4acc2833
[2] https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8730254807617968321/+/u/run_test.dart_for_docs_shard_and_subshard_None/stdout

@srawlins
Copy link
Member Author

srawlins commented Dec 3, 2024

Did you remove the library name from packages/flutter_driver/lib/flutter_driver.dart [1] before running dartdoc?

I did not! 🤣 thanks for the tip 😁

@srawlins
Copy link
Member Author

srawlins commented Dec 5, 2024

Hmm I'm not sure how this should work. I don't think we can strip the package name from the path, because then we could have collisions. For example, the vm_service package has a library called "utils" and it's maybe a miracle that no other package at api.flutter.dev has a top-level library named "utils".

I see on the main channel flutter docs, we have package prefixes, like:

And for a named library, we have the un-namespaced URL, just hoping it won't collide I guess, like:

It's unfortunate that a path like /flutter/package-<PACKAGE NAME>_<LIBRARY NAME>/ can expand to something like /flutter/package-leak_tracker_flutter_testing_leak_tracker_flutter_testing/ but I don't see a way around it. We could shorten URLs where the package name matches the library name, but that still leaves things like https://main-api.flutter.dev/flutter/package-flutter_web_plugins_url_strategy/ as very long.

WDYT, @goderbauer ? I think maybe we just leave libraries named if we want a short URL for them at api.flutter.dev.

@goderbauer
Copy link
Contributor

I am ok with leaving them named, but then I think the unnecessary_library_names lint is broken since the library name isn't unnecessary. It's there for docs.

@srawlins
Copy link
Member Author

We can add a comment to the unnecessary_library_names rule that says, "If you are the Flutter SDK or publish your own docs with the --auto-include-dependencies flag, and you desire a specific URL for your docs, you may not want to use this rule."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants