Use a custom Enum class for improved speed #117
Conversation
|
Hi Gobot, thank you for your efforts. |
There was a problem hiding this comment.
Thank you for helping out.
As you know I'm not much convinced to the practical benefits of faster Enums.
If you could cover the new Enum class with unit tests, so it is clear what features are supported, I would be confident enough that it wouldn't break anything so that we can merge it nonetheless.
My two concerns are
- extra mental load on maintainers and users who, in the course of encountering or fixing bugs (of which we have plenty still, also related to enums), will consider the Enum class as a potential root cause, and spend extra time going through and debugging the Enum code.
We can avoid this by guaranteeing that our custom Enum implementation behaves 100% the same as the Python one. Full test coverage seems essential in my opinion. - Incompatibility with how users were applying betterproto Enums before. Again tests can rule this out to some extent. I did a quick search to see how SQL Alchemy uses Enums for example, but it turns out they don't natively support it, and the same goes for Django forms. Seems that Enum support is not widespread so that lowers concerns for compatibility with other libraries.
For tests, we can probably reuse most tests from the cpython:
betterproto/__init__.py
Outdated
| @@ -776,7 +757,7 @@ def FromString(cls: Type[T], data: bytes) -> T: | |||
| return cls().parse(data) | |||
|
|
|||
| def to_dict( | |||
| self, casing: Casing = Casing.CAMEL, include_default_values: bool = False | |||
| self, casing: Optional[Casing] = None, include_default_values: bool = False | |||
There was a problem hiding this comment.
Since Enums are not mutable, do we need this change? Or was passing a default not possible with the new class?
I think it was helpful for developers to see the default casing from the parameter.
There was a problem hiding this comment.
Unfortunately it raises an AttributeError as the class hasn't been created yet.
There was a problem hiding this comment.
Actually, I guess we don't need to use Betterproto Enums for Casing. Perhaps just keep things simple and use the python Enum instead?
There was a problem hiding this comment.
I don't think it's that big of a deal, if someone wants to see the default they can just view source.
|
Everything should now work |
|
This is a bad idea, making these consistent is too much of a pain, sticking with the current implementation is fine. |
Significant speed improvements over the builtin Enum class.
Full tests here https://gist.github.com/Gobot1234/e3883e9db151ce3b33adf4d38c140936
Fixes a bug discussed on slack relating to
to_dictandIndexErrorswhen using Enums.