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

Adding support for int and bool EnumProperties #259

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ejmount
Copy link

@ejmount ejmount commented Mar 13, 2023

What it says on the tin. I was looking for this functionality for my own project and found stubs for it, so I've filled in the functionality and updated tests/docs accordingly. cargo test comes back all green after my changes.

Copy link
Owner

@Peternator7 Peternator7 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 really great, and I know a couple have people have asked about this feature. Just a couple minor structure things and I think we can get this merged.

strum/src/lib.rs Outdated
fn get_int(&self, _prop: &str) -> Option<usize> {
Option::None
}
fn get_int(&self, prop: &str) -> Option<usize>;
Copy link
Owner

Choose a reason for hiding this comment

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

get_usize is probably a more future-proof name

Copy link
Author

@ejmount ejmount Apr 3, 2023

Choose a reason for hiding this comment

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

I agree and I'm happy to make that change, but I also thought it'd be worth mentioning that while it'll be more future proof, it's also technically a breaking change. I'd be immensely surprised if any real code got broken, but I just wanted to check you're ok with that.

Copy link
Author

Choose a reason for hiding this comment

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

I jinxed myself by saying earlier nothing would break, because there seems to be an odd dependency between strum_macros and a previous version of the main package. I've made the change and updated the Cargo.toml to use the local version, which solved the issue.

Copy link
Owner

Choose a reason for hiding this comment

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

Hey @ejmount, sorry for the delay getting back to you. I've been out of town for a few weeks. I missed that I had stubbed that method out at some point in the past. If you're still interested in completing this, here's what we can do. I can publish a new version of strum (not macros) with an updated trait definition then we can rebase your changes onto the new version.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it sounds like that's still not supported, so it'd be great if you could publish a new version to make that update.

strum_macros/src/helpers/metadata.rs Outdated Show resolved Hide resolved
@@ -26,4 +26,4 @@ rustversion = "1.0"
syn = { version = "1.0", features = ["parsing", "extra-traits"] }

[dev-dependencies]
strum = "0.24"
strum = { path = "../strum" }
Copy link
Owner

Choose a reason for hiding this comment

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

I don't believe you can publish to crates.io with a path dependency.

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

2 participants