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

proposal: consider a unnecessary_library_names lint #58950

Closed
pq opened this issue Dec 6, 2022 · 7 comments
Closed

proposal: consider a unnecessary_library_names lint #58950

pq opened this issue Dec 6, 2022 · 7 comments
Assignees
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-proposal linter-set-recommended P3 A lower priority bug or feature request

Comments

@pq
Copy link
Member

pq commented Dec 6, 2022

Current guidance discourages named libraries (dart-lang/site-www@dbac48c) and we should consider a lint to support the advice.

Related lints:

/fyi @MaryaBelanger @munificent @srawlins

@MaryaBelanger
Copy link
Contributor

My take:

  1. library_names shouldn't be a lint anymore
    (it conflicts or at least confuses "no named libraries" if we also promote a "right" way to name libraries)
  2. Add a new lint called unnecessary_library_name

Something like this:

const _name = 'unnecessary_library_name';

const _desc =
    'Avoid library names.';

const _details = r'''
From the [style guide](https://dart.dev/guides/language/effective-dart/style#dont-explicitly-name-libraries):

**DON'T** explicitly name libraries by attaching a name to a library directive.

**BAD:**
library my_library;

**GOOD:**
library;

''';
// ....

class UnnecessaryLibraryName extends LintRule {
  static const LintCode code = LintCode(
    _name,
    'Library directives should not have names.',
    correctionMessage: 'Try deleting the library name.',
  );

(not set on From the [style guide]... but that's a different conversation)

@pq
Copy link
Member Author

pq commented Jan 5, 2023

I think we're in agreement so I'm bumping this to "accepted". Chime in if you have reservations!

@bwilkerson
Copy link
Member

I also prefer the name unnecessary_library_name.

But I have a question: will the lint still fire if the library has parts and at least one of the parts uses the name of the library?

I'd kind of prefer that the answer be "yes", and we can easily automate converting the part-of directive to use a URL, but it is harder for users to update manually and there are code generators that might still be using names.

I'd like to see a more complete specification before we mark this as accepted.

@pq pq changed the title proposal: consider a no_library_names lint proposal: consider a unnecessary_library_names lint Jan 5, 2023
@pq pq modified the milestones: Dart 3 beta 1, Dart 3 beta 2 Feb 2, 2023
@pq pq modified the milestones: Dart 3 beta 2, Dart 3 beta 3 Mar 13, 2023
@pq pq added the P3 A lower priority bug or feature request label May 22, 2023
@natebosch
Copy link
Member

natebosch commented Feb 8, 2024

will the lint still fire if the library has parts and at least one of the parts uses the name of the library?

I don't think we care too much either way. We already have https://dart.dev/tools/linter-rules/use_string_in_part_of_directives in our core set and any project which is conformant to this one will not be impacted either way by this decision.

Whichever is easier to implement should be fine. If it's easy enough to allow library names when there is a part relying on that name I'd lean towards doing that since it might be less confusing in edge cases, but I wouldn't bother if it's significantly more effort.

@lrhn
Copy link
Member

lrhn commented Feb 8, 2024

I'd be fine with just saying "no library names" and not checking for whether they are used.
If one really needs a library name for something, // ignore: works.

Possible definition:

Don't have a library name in a library declaration.

Library names are not necessary.

A library does not need a library declaration, but one can be added to attach
library documentation and library metadata to.
A declaration of library; is sufficient for those uses.

The only use of a library name is for a part file to refer back to its owning library,
but part files should prefer to use a string URI to refer back to the library file, not a library name.

If a library name is added to a library declaration, it introduces the risk of name conflicts.
It's a compile-time error if two libraries in the same program have the same library name.
To avoid that, library names tend to be long, including the package name and path,
just to avoid accidental name clashes. That makes such library names hard to read,
and not even useful as documentation.

Good:

/// This library is awesome.
library;

part "apart.dart"; // contains: `part of "good_library.dart";`

Bad

/// This library has a long name.
library magnificator.src.helper.bananas;
library utils; // Not as verbose, but risks conflicts.

When it comes to library names, the only winning move is not to play.

@pq pq self-assigned this Mar 19, 2024
@pq
Copy link
Member Author

pq commented Mar 19, 2024

It looks like we're shooting to include this in the next package:lints release, so I'll take a crack at implementing.

copybara-service bot referenced this issue Mar 20, 2024
Fixes: https://github.com/dart-lang/linter/issues/3882

(I'll follow-up with a quick-fix.)

Change-Id: Ice88bfb59a16163b003ce94c73e057d2fd968a9d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/358561
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
@pq
Copy link
Member Author

pq commented Mar 20, 2024

Implemented w/ 7145659

Quick-fix in flight: https://dart-review.googlesource.com/c/sdk/+/358681

@pq pq closed this as completed Mar 20, 2024
@devoncarew devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-lint-proposal linter-set-recommended P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

6 participants