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

Consider adding unnecessary_library_name to recommended #181

Open
devoncarew opened this issue Mar 20, 2024 · 10 comments
Open

Consider adding unnecessary_library_name to recommended #181

devoncarew opened this issue Mar 20, 2024 · 10 comments

Comments

@devoncarew
Copy link
Member

devoncarew commented Mar 20, 2024

This is a proposal to add unnecessary_library_name to the recommended set.

This very newly added lint landed in https://dart-review.googlesource.com/c/sdk/+/358561.

It fires whenever there's a library name in a library directive; this matches our recommendation in recommended Dart: "DON'T have a library name in a library declaration".

Note that this will not be generally available until 3.4.0 is stable.

Implicit in this request is that we remove package_names from the recommended set ("Use lowercase_with_underscores for package names.").

@devoncarew devoncarew changed the title Add unnecessary_library_names to recommended Consider adding unnecessary_library_names to recommended Mar 20, 2024
@devoncarew
Copy link
Member Author

cc @lrhn, @natebosch, and @goderbauer for review

@goderbauer
Copy link
Contributor

I am sympathetic to adding this to recommended eventually. Since it is very hot of the press, I would prefer letting it bake a little bit to shake out any potential bugs or unintended side effects. As part of that I'd like to dogfood it in the flutter/flutter repository first before we put this into recommended.

@natebosch
Copy link
Member

I'm in favor of adding it - I don't think the risk for side effects is very high. I don't think it's urgent though, so I would be fine holding back for a short while to try it in a few repos.

@pq pq changed the title Consider adding unnecessary_library_names to recommended Consider adding unnecessary_library_name to recommended Mar 20, 2024
@devoncarew
Copy link
Member Author

Based in the comments above, lets delay adding this to the ruleset until the lint's been:

  • available from the stable sdk
  • we've tried it on a few larger / representative codebases

We can reconsider at our next regular review.

@pq
Copy link
Member

pq commented Mar 22, 2024

I've been doing some testing and encourage people to do the same.

Try dart fix --code=unnecessary_library_name --apply and if you hit any snags let me know.

(I've got a fix for one in flight now: https://dart-review.googlesource.com/c/sdk/+/359321 -- don't lint if the unnamed-libraries feature is not enabled.)

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Mar 22, 2024
Fixes generated using `dart fix --code=unnecessary_library_name --apply`

See conversation in dart-lang/lints#181

Change-Id: Id2b975e4aa27348d8883f1aea22e00dd9f4fc493
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/359322
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Mar 25, 2024
Related to: dart-lang/lints#181

Change-Id: I4a247fa93c6659d5082d2cb6047aedc6a13a4209
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/359321
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
@goderbauer
Copy link
Contributor

goderbauer commented Mar 25, 2024

I tried enabling this on the flutter repository (flutter/flutter#145714) and ran into a surprising side effect: Dartdoc appears to depend on the unnecessary library names and uses them to structure the generated doc. With an unnecessary library name present (e.g. library material;) it generates nice URL paths like flutter/material/TextField-class.html. When I remove the unnecessary name (just library;) it generates the rather ugly package-flutter_material/TextField-class.html path for the same entity. This one is even worse:

flutter/material/material-library.html (with unnecessary library name present)
package-flutter_material/package-flutter_material-library.html. (without)

So, turning this on and enforcing it on Flutter would invalidate lots of external links (e.g. from stackoverflow) pointing to our docs. The same is probably also true for many other packages (I saw that package:intl for example could also be affected if the library names are removed there).

It is not clear to me why dartdoc behaves differently based on the presence of a library names. I wonder if we can change dartdoc to generate the same nicer paths based on the implicit library name when no explicit name is given before we roll out this lint further? This would likely also keep most existing external links functional. (cc @srawlins)

@devoncarew
Copy link
Member Author

I suspect we do want to update dartdoc to not rely on the library name to decide what the generated file paths should look like - that should be based solely on the path of the defining source library (e.g., package:flutter/material.dart).

@pq
Copy link
Member

pq commented Mar 25, 2024

/fyi @kallentu

@srawlins
Copy link
Member

An update to generate all paths one way or to generate paths the other way, regardless of library name, would break links. 😁

But we can update dartdoc such that:

  • for named libraries, generate the nice path,
  • for unnamed libraries, generate the nice path, and a redirect HTML file at the less nice path, pointing the old to the new.

See also dart-lang/dartdoc#1346, where I could similarly leave client-side redirects in place.

Alternatively, if there is some open standard (like Apache httpd?) of server-side permanent redirect specifications, we could add a feature to dartdoc to generate such a "redirect file." I guess the important case is whatever is serving api.flutter, api.dart, and pub.dev.

@goderbauer
Copy link
Contributor

I like that suggestion. 👍

The flutter API docs are hosted on Firebase. Looks like the support configuring redirects like this: https://firebase.google.com/docs/hosting/full-config#redirects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Triage backlog
Development

No branches or pull requests

5 participants