Adding support for int and bool EnumProperties #259
Conversation
| fn get_int(&self, _prop: &str) -> Option<usize> { | ||
| Option::None | ||
| } | ||
| fn get_int(&self, prop: &str) -> Option<usize>; |
There was a problem hiding this comment.
get_usize is probably a more future-proof name
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/Cargo.toml
Outdated
| @@ -26,4 +26,4 @@ rustversion = "1.0" | |||
| syn = { version = "1.0", features = ["parsing", "extra-traits"] } | |||
|
|
|||
| [dev-dependencies] | |||
| strum = "0.24" | |||
| strum = { path = "../strum" } | |||
There was a problem hiding this comment.
I don't believe you can publish to crates.io with a path dependency.
|
So this came back to my attention recently so I've rebased it against the current release and resolved conflicts. I had another look at the dependency issue, and as far as I can tell everything works correctly if the name remains The version I've uploaded just now supports ints and bools and passes local tests, and IMO would be ready to merge if you're OK with I'm not sure why the CI tests are failing on some platforms and not others, but the issue doesn't appear to relate to this change - has the MSRV changed since the pipeline was last updated? |
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 testcomes back all green after my changes.