-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add support for IntBackedEnum #33
Comments
@DaanBiesterbos thank you for bringing this issue to my attention. You’ve raised some valid concerns regarding the current implementation’s dependency on enum order and the challenges with migrating pre-existing bitmasks. I agree that relying on the order within the enum introduces potential risks, especially in cases where permissions or other critical flags are involved. The idea of adding placeholders to maintain the correct bitmask values is indeed not an ideal solution. Your proposed solution of supporting int-backed enums makes sense. It would provide more flexibility and mitigate the risks associated with the enum order, allowing for a safer and more predictable implementation. I believe this enhancement would make the library more robust and easier to integrate with existing codebases. If you're willing to create a pull request, I would greatly appreciate it. At the moment, I’m dealing with some time constraints, but I’ll make sure to review the PR as soon as I have the opportunity. Thanks again for your valuable input and contribution! |
Sure, no problem! :) What I ended up doing was to create a new class. It was difficult to add a constructor argument because the bitmask is instantiated in places where I don't know the value of the new argument. Since we only need my solution I could simply check if the enum is an int backed enum, and if so, use the value. But that won't do, because it would not be backward compatible. To keep the bundle backward compatible I would say it is best to make a subclass (IntBackedEnumBitmask) and perhaps an abstract class to make it easier for developer to implement their own implementation. I did find one quirky little thing, which is that the is_a() and is_subclass_of() functions don't seem to work for enums and the IntBackedEnum interface. Possibly the interface is just a stub and is not actually exposed to user land. I had to use reflection to validate that a given FQCN is indeed an int backed enum. Not the prettiest but that's what I could come up with.
Do you overall agree to this approach? Using a subclass and the reflection solution to validate the FQCN argument? |
@DaanBiesterbos Thank you for sharing your approach. I appreciate the effort to maintain backward compatibility, but I have some concerns about using reflection in this case. While reflection can be powerful, I believe it's best to avoid it when simpler solutions exist. Instead, I would suggest first checking if the enum is a non-empty |
Right now the order and position within the enum determine the result. This introduces some problems.
Bitmasks are often used to check permissions etc, therefore the minor risks should be taken more seriously I believe.
Right now I would have to add placeholders to use this bundle without changing the bitmask itself.
Obviously, this is not what I need, even more so when more values are missing.
Proposed solution:
Add support for int backed enums. Optionally add an argument to the EnumBitmask constructor.
For example:
For now I'll just implement my own EnumBitmask because I cannot wait for this change.
If you agree to the solution I don't mind to create a pull request for it.
The text was updated successfully, but these errors were encountered: