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 rule to report types that should likely not be exposed to the user #22

Open
jfmengels opened this issue Jul 25, 2024 · 0 comments
Open

Comments

@jfmengels
Copy link
Owner

What the rule should do:

Report when a type is exposed as part of a package but shouldn't be, because it's either not used or only used in the internals.

What problems does it solve:

Here's an example: In this package, the type CustomError is exposed but never used in external APIs. And the type is on itself not useful, especially since it's opaque.

Its presence is unnecessary and confusing, and therefore we should remove.
However removing it is a breaking change which makes catching this early really important, and an automated check could therefore prove quite useful.

Example of things the rule would (not) report:

I think there are two things we could report.

The first one - which is the safe one - is to report opaque types (or, indirectly, aliases to opaque types) that are never referenced in the public API. We could reuse the logic present in NoMissingTypeExpose for that I believe.

If a type is opaque and never generated through an API, then it can't be useful for anything, and therefore it should be removed.

The only exception I can think of is if it is to be used as a phantom type, but I believe that in that case, we should at least see one reference to the type in the public API.

The second one - which I have more doubts about - is reporting non-opaque types, which again are unreferenced anywhere else. I believe that this could catch a few things, but it can lead to a few false positives when the type has inherent value (for instance a list of countries/units/... like https://package.elm-lang.org/packages/rl-king/elm-iso3166-country-codes/latest/Iso3166#CountryCode).

In this case, either we should allow configuration or advise not using it. I'm unsure yet, without more experimentation.

Since it's possible that someone has already published a package with an error like this, I think it could make sense to have an exception list in the rule's configuration. Either that or promote the use of elm-review suppress for this rule.

If the rule often gives false positives, then I think an exception list could be configured, though probably not initially.

When (not) to enable this rule:

This would only be useful for packages only, but probably for all packages.

I am looking for:

  • Feedback
  • A name for the rule
  • Someone to implement it with/for me
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant