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

Improve display text for libraries #3552

Merged
merged 4 commits into from
Nov 2, 2023

Conversation

srawlins
Copy link
Member

@srawlins srawlins commented Oct 25, 2023

Fixes #1658 and fixes #1000.

The changes in this PR do not ever print the full import URI (like package:googleapis/admob/v1.dart) or even the import path with the .dart extension (like admob/v1.dart), except in the breadcrumb. Mainly this PR uses the basename of the library with its directory path after lib/ (like admob/v1). Here are some screenshots:

Screenshot 2023-10-25 at 11 17 44 AM
Screenshot 2023-11-01 at 10 20 47 PM
Screenshot 2023-11-01 at 10 21 04 PM
Screenshot 2023-10-25 at 11 54 47 AM
Screenshot 2023-11-02 at 1 13 59 PM
Screenshot 2023-11-02 at 1 14 08 PM

Additionally:

  • Make Library.prefixToLibrary private.
  • Refactor Library.dirName to just inline all of its helper methods. I think it is much more readable this way.
  • Remove Library.nameFromPath (as part of the above).

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@devoncarew
Copy link
Member

For my 0.02, I do think it's useful to show the file extension (admob/v1.dart instead of admob/v1), even at the expense of some brevity. It helps the user know what they're looking at w/o having to think about it, and will be familiar with what they'd import or what they'd see in their IDE.

@srawlins
Copy link
Member Author

CC @kevmoo @goderbauer

@kevmoo
Copy link
Member

kevmoo commented Oct 25, 2023

For my 0.02, I do think it's useful to show the file extension (admob/v1.dart instead of admob/v1), even at the expense of some brevity. It helps the user know what they're looking at w/o having to think about it, and will be familiar with what they'd import or what they'd see in their IDE.

I'd actually argue LEAVE the .dart out and ship that way (maybe have it in a tooltip) and see if there are complains!

@srawlins
Copy link
Member Author

@goderbauer I found a few differences in how this shows api.flutter.dev; almost zero libraries are located deeper than lib/ so this PR changes very little for Flutter. But the material_color_utilities package has some:

BEFORE

Screenshot 2023-10-25 at 1 05 25 PM

AFTER

Screenshot 2023-10-25 at 1 04 45 PM

There may be some misconceptions about src/ directories in that package's layout.

@srawlins
Copy link
Member Author

For now, there were more recommendations to leave the .dart off of library names, and I agree it kind of noises up the left sidebar for api.flutter.dev.

And yes, I really like the tooltip idea. I'd like to land that as well, separately.

@goderbauer
Copy link
Contributor

There may be some misconceptions about src/ directories in that package's layout.

I would agree with that. Given the current layout, dartdoc's output seems reasonable, though. I'll see if we can fix up the layout, but that shouldn't block this work here.

@srawlins
Copy link
Member Author

Hi @kallentu , as part of learning the code here, you could participate in code review for this PR. You can read over the two issues that are fixed by this PR and ask me any questions about the code.

@srawlins srawlins force-pushed the library-display-name branch from cd3f505 to 99e0f79 Compare October 31, 2023 22:40
lib/src/model/library.dart Show resolved Hide resolved
lib/src/model/library.dart Show resolved Hide resolved
lib/src/generator/template_data.dart Outdated Show resolved Hide resolved
@srawlins srawlins merged commit 10e2cfc into dart-lang:main Nov 2, 2023
9 checks passed
@srawlins srawlins deleted the library-display-name branch November 2, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants