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

Remove package_prefixed_library_names from core set (and deprecate) #172

Closed
parlough opened this issue Dec 18, 2023 · 8 comments · Fixed by #179
Closed

Remove package_prefixed_library_names from core set (and deprecate) #172

parlough opened this issue Dec 18, 2023 · 8 comments · Fixed by #179

Comments

@parlough
Copy link
Member

parlough commented Dec 18, 2023

I'm opening this issue in this repository to discuss how to handle ecosystem affecting linter rule changes, such as this case when one of the effected lints is in one of the core sets. (As requested by Brian)

Currently, package_prefixed_library_names which is in the core set isn't functional. The same goes for package_api_docs which isn't in any of the core sets currently. This has been the case for the past few releases after dart-lang/linter@008a8a0.

Fixing/reimplementing these lints now is a bit dangerous as they are both quite heavily depended on, especially with package_prefixed_library_names as part of the core set. It could result in a lot of new lints triggering on code with an SDK update.

So the question is if those lints starting to trigger on an SDK update are ok, or should these lints be removed from the core sets, deprecated, and slated for removal? If their functionality is still desired, we can always introduce new lints. Technically these both don't follow today's linter rule naming scheme anyway.

As an aside for these two rules specifically, but not relevant to the question of how to handle this in general:

  • I think package_prefixed_library_names isn't super useful since users should generally shouldn't name libraries anymore.
  • While not the exact same behavior, public_member_api_docs covers many of the use cases of package_api_docs.

\cc @bwilkerson @pq @devoncarew

@devoncarew devoncarew changed the title [Discussion] Remove package_prefixed_library_names from core set (and deprecate) Remove package_prefixed_library_names from core set (and deprecate) Feb 6, 2024
@github-project-automation github-project-automation bot moved this to Triage backlog in Lints Triage Feb 6, 2024
@devoncarew
Copy link
Member

@natebosch @goderbauer @lrhn - can you review this proposal and give an initial thumbs-up / thumbs-down (with some rational if useful)?

@devoncarew devoncarew moved this from Triage backlog to Under consideration in Lints Triage Feb 6, 2024
@natebosch
Copy link
Member

Is there risk of users not getting help if they add a library name unnecessarily, and the library name doesn't follow these patterns? Should we be linting to encourage unnamed libraries?

If the lint is currently not firing, I'm on board with removing it from the lint sets before (or instead of) fixing the lint implementation. I do think it's lower value now that we can have bare library; and I'd be OK with not having any lints at all around library names if we think that's easier for us.

@goderbauer
Copy link
Contributor

goderbauer commented Feb 7, 2024

Maybe we should consider unnecessary_library_directive for inclusion in recommended to cut down on library statements?

Since the lint is not working (@parlough is there a bug on file for this?) I'd be ok with removing it.

@natebosch
Copy link
Member

unnecessary_library_directive SGTM. I'd lean towards core, but I'd be fine with recommended.

@parlough
Copy link
Member Author

parlough commented Feb 7, 2024

Since the lint is not working (@parlough is there a bug on file for this?) I'd be ok with removing it.

dart-lang/sdk#58734 is the original bug. I don't have all the context, but @pq might :)

Maybe we should consider unnecessary_library_directive for inclusion in recommended to cut down on library statements?

+1 Yes please. Then dangling_library_doc_comments and library_annotations (#177) can help developers determine when they are still useful to have.

@parlough
Copy link
Member Author

parlough commented Feb 7, 2024

It sounds like there's general agreement that it's okay to remove this lint because it doesn't work, but there's also some thoughts that the lint isn't super valuable anymore.

Considering that, I'd propose the following:

  1. Remove package_prefixed_library_names from package:lints
  2. Add unnecessary_library_directive to the core lints
  3. Deprecate package_prefixed_library_names in the SDK
  4. Implement unnecessary_library_names (proposal: consider a unnecessary_library_names lint sdk#58950) which mostly supersedes this lint.
  5. Add unnecessary_library_name to the core lints once available in a stable SDK :)

@lrhn
Copy link
Member

lrhn commented Feb 7, 2024

If you do give your library a name (why?) then I think it is a good idea to include your package name in the library name.

I just wrote library pkg.packageName.impl;, and I prefer that to just prefixing with the package name alone, because I don't necessarily expect pub packages to be the only kind of code.
(I needed the library name for a hack including parts and conditional imports, which could be better handled by conditional augments).

Ok with removing the lint.
Ok with unnecessary_library_name.
Ok with removing library names from the language (when we get conditional augments).

@devoncarew
Copy link
Member

After discussion, we're in favor of removing this lint from the core set. The other items discussed in this issue (unnecessary_library_directive, creating a unnecessary_library_name lint, ...) will be followed up on elsewhere.

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

Successfully merging a pull request may close this issue.

5 participants