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

Add unnecessary_library_directive #178

Closed
devoncarew opened this issue Feb 7, 2024 · 6 comments
Closed

Add unnecessary_library_directive #178

devoncarew opened this issue Feb 7, 2024 · 6 comments

Comments

@devoncarew
Copy link
Member

Avoid library directives unless they have documentation comments or annotations.

Creating this issue as a companion to #172 (see comments #172 (comment), #172 (comment)).

This lint - along with a few others - would help with general library statement hygiene.

@devoncarew
Copy link
Member Author

@natebosch @goderbauer @lrhn - can you weigh in here (even if you have already on #172)?

cc @parlough

@devoncarew devoncarew moved this from Triage backlog to Under consideration in Lints Triage Feb 7, 2024
@goderbauer
Copy link
Contributor

I am in favor of adding this lint. My suggestion would be to add it to recommended since IMO it does not "help identify critical issues that are likely to lead to problems when running or consuming Dart code", which has been our bar for core lints.

@lrhn
Copy link
Member

lrhn commented Feb 7, 2024

Link: https://dart.dev/tools/linter-rules/unnecessary_library_directive

It's not that bad. It's not great.
I really can't bring myself to care whether people write library; at the top of their library.

I'm fine with putting it in google internal lints, or Dart/Flutter team lints, where people care a lot about conformance to a single style.
Optional lint for people who want it, sure. Recommended or core, not worth it.

I don't want to bother our users with this, because it really doesn't matter.
It's not about telling people that you can remove this library declaration. Giving a warning is telling them that they should. And they really don't need to, by any reasonable measure.

Recommended lints should have significant value. We are bothering our users about something, so that better be meaningful. It better make their code better in at least some way.

This is just noise. Nobody's day gets better because we make them remove library;.

A library author can always remove the library; if it bothers them. If it doesn't, it shouldn't bother us, and we shuldn't bother them.

@natebosch
Copy link
Member

I think removing noise is enough justification for recommended. I'm sold on keeping it out of core.

This is another one which might be best as a transient lint. In the absence of that capability I think users of the recommended set would benefit enough to make it worth including.

I still see packages come in to google3 with unnecessary library names that look to me like a habit of copy/paste. If we recommend a lint that requires library; without names I think this one goes along well with it.

@lrhn
Copy link
Member

lrhn commented Feb 8, 2024

removing noise

To be precise, I was considering the lint to be the noise, not the library; declaration.

Warnings about things that don't actually matter is worse than having things that don't actually matter.
I know it's a fine line to draw between "unnecessary code" and this, but the reason we generally discourage unnecessary code isn't that it's inherently bad, but that it's potentially confusing. A reader may see something which is more complicated than necessary, and start looking for a reason where there is none. That's a cost on the reader.

I expect the cost on someone seeing a library; with no comment or annotation to be exactly zero. There is nothing to be confused about.
Showing a warning that you should remove the library; is a cost, no matter how small, and with a straight zero benfit I don't think it's worth it.

There is just no value at a normal user, or future readers of their same code, from telling them to remove a library; declaration.
If it doesn't bother them, that's all that matters. If it does, I think they'll know to remove it.

I don't know what a transient lint is, but if it's something that basically says "FYI, the thing you just changed can be improved as ....", and then goes away if the author doesn't do anything about it (like they save the file two more times without editing near the code, or commits it to git), then that would be fine.
The message would be formatted as something a reader would recognize as transient. "If I ignore this, it'll go away." And huzzah, it did. That would be fine.

A persistent nag about something unnecessary (and possibly deliberate, if planning to come back and write comments) isn't fine, IMO.

@devoncarew
Copy link
Member Author

After discussion, we're not going to add this to a package:lints lint set. We're not currently willing to lint if there's just a bare library; directive (one w/o doc comments or annotations).

However, we do think its worth linting if there's an unnecessary library name (as per effective dart). See the proposal for that here: dart-lang/linter#3882.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants