cargo-semver-checks icon indicating copy to clipboard operation
cargo-semver-checks copied to clipboard

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

Open obi1kenobi opened this issue 2 years ago • 4 comments

Per https://github.com/obi1kenobi/cargo-semver-checks/discussions/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 avatar Jan 20 '23 19:01 obi1kenobi

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?

epage avatar Jan 20 '23 19:01 epage

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.

obi1kenobi avatar Jan 20 '23 19:01 obi1kenobi

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.

epage avatar Jan 20 '23 20:01 epage

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.

obi1kenobi avatar Jan 20 '23 20:01 obi1kenobi