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

Made edition an enum again #185

Merged
merged 5 commits into from Jul 1, 2022
Merged

Conversation

morr0ne
Copy link
Contributor

@morr0ne morr0ne commented Jun 30, 2022

As mentioned in #183 this makes the Edition an enum again but also future proofs it by adding an Unknown variant

@morr0ne
Copy link
Contributor Author

morr0ne commented Jun 30, 2022

This should be ready to merge but it would be a good idea to add some tests against all the enum variants. Right now only edition 2018 is tested. That ought to be enough but one can never be too sure. I'd love @oli-obk opinion on the tests as I'm a bit confused as how they are organized what the best way to test them would be.

@morr0ne morr0ne mentioned this pull request Jun 30, 2022
@sunshowers
Copy link
Contributor

As I mentioned in #183, I'm afraid this has fundamental issues as explained in https://sunshowers.io/posts/open-closed-universes/#a-pattern-to-avoid-unknown-or-other-variants

@morr0ne
Copy link
Contributor Author

morr0ne commented Jun 30, 2022

As highlighted by @sunshowers in #183 and their blog post this may have caused some issue when comparing existing values so I manually Implemented Eq/PartialEq/Hash so that cases like:

Edition::E2018 == Edition::Unknown("2018")

produce the correct value

@jplatte
Copy link

jplatte commented Jul 1, 2022

I think it would be better if Unknown was #[doc(hidden)] _Unknown(UnknownEdition) where UnknownEdition is a struct with a private string field. Then you can add a AsRef<str> impl and maybe .as_str() method for people to use when they want to check for unknown editions.

@morr0ne
Copy link
Contributor Author

morr0ne commented Jul 1, 2022

@jplatte I appreciate the input but most of the discussion has happened on #183, that might be a good idea but I think we might just end up discarding this whole pull request.

@morr0ne
Copy link
Contributor Author

morr0ne commented Jul 1, 2022

I reverted back to the original implementation while also including editions up to 2030. I think 8 years should be more that enough considering that by that time the concept of editions might not even be relevant anymore.

src/lib.rs Show resolved Hide resolved
morr0ne added a commit to morr0ne/cargo_metadata that referenced this pull request Jul 1, 2022
@oli-obk
Copy link
Owner

oli-obk commented Jul 1, 2022

Lgtm, i'll wait for @sunshowers to chime in before merging though

@sunshowers
Copy link
Contributor

Looks good to me, thanks for this!

@oli-obk oli-obk merged commit b8df928 into oli-obk:main Jul 1, 2022
@morr0ne morr0ne deleted the fix-edition-enum branch July 2, 2022 06:30
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

Successfully merging this pull request may close these issues.

None yet

4 participants