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

chore: remove derive_is_variant #77

Closed
wants to merge 0 commits into from
Closed

Conversation

mokurin000
Copy link
Contributor

@mokurin000 mokurin000 commented Feb 22, 2024

derive_is_variant has not been update for several years, while strum is under active developement

close #76

btw my script to check dependencies imported in multiple versions:

import toml

lock = toml.load("Cargo.lock")
packages = lock['package']
names = {}

for package in packages:
    name = package['name']
    version = package['version']
    versions = names.get(name, [])
    versions.append(version)
    names[name] = versions

for name, versions in names.items():
    if len(versions) == 1:
        continue
    print(name, versions)

Copy link
Owner

@Mange Mange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks cool and all, but is_f64 changing to is_f_64 is a breaking change.

Strum does not offer custom naming in their derive macro, so I think the play here is to manually add all the is_xn methods and delegate to the is_x_n version.

Perhaps even mark the old names as deprecated or something.


I think I should open a feature request on strum to ask for

#[derive(EnumIs)]
pub enum Foo {
  Baz, // is_baz

  #[strum(name = "pub")]
  Bar, // is_pub
}

You could also do that, if you wish. I don't have time for it right now, sadly.

@Mange
Copy link
Owner

Mange commented Feb 28, 2024

Eh, I added a PR to strum to get custom names. I found the time.

Peternator7/strum#336

Let's wait for this before progressing on this PR.

@mokurin000
Copy link
Contributor Author

mokurin000 commented Apr 1, 2024

@Mange how if we provide is_f64 that calling self.is_f_64()
until users defined is_f_64 in their traits for Value (and in this case they should check out fully-qualified-syntax), this won't be a breaking change

strum ignored the rename support PR for weeks

@Mange
Copy link
Owner

Mange commented Apr 1, 2024

@Mange how if we provide is_f64 that calling self.is_f_64() […]

I am fine with this, but I really want to make sure we can mark the generated is_f_64 as being deprecated.

[…] until users defined is_f_64 in their traits for Value, this won't be a breaking change […]

No, if we add new methods (is_f_64), then removing those becomes the breaking change instead. I'm not worried about people implementing Value themselves, only that they try to use Value in their own code and use is_f_64/is_f64 in a way that breaks during a version upgrade.

I don't want duplicated methods like this. Note that this is for all the number types, not just f64.

strum ignored the rename support PR for weeks

Yeah, it's really unfortunate… I suspect that maintainer has a lot going on right now.


Maybe the best way forward is to remove derive_is_variant (the problem in question) and manually implementing the is_X methods without strum or any other macro. It's not too hard, honestly. I bet ChatGPT or Copilot could whip them out pretty quickly after one or two of them are already implemented.

I could do it if you don't feel motivated to do it yourself. :-)

@mokurin000 mokurin000 changed the title chore: migrate to strum::EnumIs chore: remove derive_is_variant Apr 2, 2024
@mokurin000 mokurin000 closed this Apr 2, 2024
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.

drop derive_is_enum_variant dependency
2 participants