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

Implement get_int/get_bool for properties #391

Merged

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Nov 20, 2024

Fixes: #313

I am also suggesting to use i64 as return type for get_int in order to cover also negative numbers.

@pali6
Copy link

pali6 commented Nov 20, 2024

It might be a good idea to instead make it so each property carries its own type based on the syntax used to declare it (e.g. int properties declared as foo = 42 as opposed to foo = "42"). That way there could be compile-time checks and there would be lesser risk of typos or of calling get_int on a string property.

@Peternator7
Copy link
Owner

Hey @marxin, thanks for the PR! This has been requested for quite a while. I tend to agree with @pali6 though that this would probably be better if accepted literals of the expected type rather than parsing the string.

In some ideal world, we could accept an expression #[strum(props(my_prop = true || false))] but that's difficult because we'd need to figure out the type. I think a better solution in the first version is to change the grammar so that Prop parses a Lit rather than a LitStr then we can do a pattern match in the macro to figure out what type of property we're looking at.

@marxin marxin force-pushed the implement-get_int-get_bool-for-props branch from 8713af6 to 0fa010d Compare November 27, 2024 22:06
@marxin
Copy link
Contributor Author

marxin commented Nov 27, 2024

Here we go. I've implemented the suggested approach for the two functions. Eventually, I decided to track the 3 categories using HashMap in enum_properties_inner, but using arrays (the original commit in this PR) might be possible. Which approach do you prefer?

fn get_bool(&self, _prop: &str) -> Option<bool> {
Option::None
}
fn get_int(&self, _prop: &str) -> Option<i64>;
Copy link

Choose a reason for hiding this comment

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

Technically this is an API break. But also the old get_int did nothing so most likely no one used it and it is okay.

@Peternator7 Peternator7 merged commit 3408033 into Peternator7:master Dec 15, 2024
1 check failed
@Peternator7
Copy link
Owner

Looks great to me! thanks for taking the time to work on this :)

@marxin marxin deleted the implement-get_int-get_bool-for-props branch December 16, 2024 05:19
@marxin
Copy link
Contributor Author

marxin commented Dec 16, 2024

Thanks for merging. May I ask you whether you plan to choke a new release any time soon?

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.

get_bool and get_int in EnumProperty are undocumented and always return None
3 participants