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

Split up "enum removed" from "enum changed to struct/union" checking #302

Open
2 tasks
Tracked by #313
obi1kenobi opened this issue Jan 20, 2023 · 4 comments
Open
2 tasks
Tracked by #313
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@obi1kenobi
Copy link
Owner

Per #296, it's confusing that changing an enum to a struct (or another type) causes a lint that states the enum was removed. While technically correct (the enum doesn't exist), it doesn't match the user's probable mental model.

Resolution:

  • Make the enum_removed lint specifically look for enum deletions, and make it not match enum -> struct or enum -> union cases.
  • Add a new enum_became_another_kind_of_type lint for changing enums to structs or unions.

Analogous changes to struct_removed should be considered as part of fixing #297 as well. The future union_removed lint should follow suit too.

@obi1kenobi obi1kenobi added A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Jan 20, 2023
@epage
Copy link
Collaborator

epage commented Jan 20, 2023

Do we need specialized struct/enum removal lints or should we just have an "api item removal" lint and then other lints for changes in the shape of that API item?

@obi1kenobi
Copy link
Owner Author

Without schema changes, struct/enum/union can certainly be grouped together (via ImplOwner), if you think that's worth doing. With a small schema change, other importables like fn and trait can be added to that list as well (making Importable be a subtype of Item instead of a parallel interface like it is now).

They are currently separate to allow us flexibility without code complexity when phrasing the user-facing error messages, which are entirely template-based.

What would be a huge help is to decide on the desirable error message(s) the user should see in each case. Then we'll know exactly what flexibility we need, and can merge the lints or not merge them, as appropriate.

@epage
Copy link
Collaborator

epage commented Jan 20, 2023

With a small schema change, other importables like fn and trait can be added to that list

This reminds me of an interesting angle to this: we'd probably want to group API items by which name universe they live in.

@obi1kenobi
Copy link
Owner Author

obi1kenobi commented Jan 20, 2023

I like the idea! Certainly worth a shot and I think it should be doable.

I also just realized that we may want to also have a grouped semver-minor check for item additions as well, instead of implementing those separately too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

No branches or pull requests

2 participants