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

feat: parse enums by value #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sanzoghenzo
Copy link

This will parse the Enums either by name or by value.

I also moved the tests outside the source and used pytest's parametrize decorator to make each bad_inp item into a separate test

closes #2

@Zaharid
Copy link
Owner

Zaharid commented Mar 14, 2022

Thank you for the contribution. I will have to sleep on this I think:

Python enums have the duplicity between names and values which seem somewhat redundant and create problems everywhere were I have used them. I wish the default enum.Enum class didn't have both names and values.

The reasoning for using names was that they seemed much more natural for things like enum.auto, particularly so for enum.Flag. Pydantic instead does the opposite and matches on values only (and hence e.g. flags don't work particularly neatly). There are other reasons why names seem preferable, such as being what is shown in the repr of the class.

The proposed option is to try both things, which is certainly possible, but is a minor departure from the "simplicity of implementation" goal. For one I would like to see if except (ValueError, TypeError) is the optimal way to implement the test. In particular names are simpler in that they must be strings, but values can be anything and so might have to go over parse_obj as well.

On a different note, I would prefer if tests remained inside the module. For one this allows to quickly check that it works when installing on a new machine (or e.g for a different python version) without having to go to the repository.

@sanzoghenzo
Copy link
Author

sanzoghenzo commented Mar 15, 2022

The reasoning for using names was that they seemed much more natural for things like enum.auto, particularly so for enum.Flag. [...] There are other reasons why names seem preferable, such as being what is shown in the repr of the class.

I understand your concerns.
Coming from VBA (a long time ago) and using also C#, I was accustomed to pass the integer values through the com interface (this is useful in excel to create a dynamic link to other apps without pinning the reference); the name/string representation are only hints for the developer.

But I see that in python, especially with auto and Flag, one would prefer to use the name.

EDIT: what about Flag combinations? The last 4 test cases below return NotAsEnumItemError, but they represent allowed values for this kind of enum.
My version doesn't solve this problem (test cases still raise the exception), but pass if I use values 1 through 7.

class Attributes(enum.Flag):
    READ = enum.auto()
    WRITE = enum.auto()
    EXECUTE = enum.auto()
    RW = READ | WRITE
    RX = READ | EXECUTE


flag_enum_tests =  [
    ["READ", Attributes.READ],
    ["WRITE", Attributes.WRITE], 
    ["EXECUTE", Attributes.EXECUTE],
    ["RW", Attributes.RW],
    ["RX", Attributes.RX],
    ["READ|WRITE", Attributes.RW],
    ["READ|EXECUTE", Attributes.RX],
    ["READ|WRITE|EXECUTE", Attributes.READ | Attributes.WRITE | Attributes.EXECUTE],
    ["RX|EXECUTE|RW|WRITE|READ", Attributes.READ | Attributes.WRITE | Attributes.EXECUTE],
]


@pytest.mark.parametrize(("value", "expected"), flag_enum_tests)
def test_flag_enum(value, expected) -> None:
    """Enums are built also from value."""
    assert parse_input(value, Attributes) == expected

For one I would like to see if except (ValueError, TypeError) is the optimal way to implement the test. In particular names are simpler in that they must be strings, but values can be anything and so might have to go over parse_obj as well.

You're right, I will add some more tests.
EDIT: I think I'm biased toward using only integers (and sometimes strings) as values, so I fail to see the benefit of an enum could having values of other types, especially if this is used in a public facing API that needs to be de/serialized (this is my use case, maybe I'm not seeing the big picture here).
Do you think there are many cases like this?

On a different note, I would prefer if tests remained inside the module. For one this allows to quickly check that it works when installing on a new machine (or e.g for a different python version) without having to go to the repository.

To me CI/CD (github actions) should take care of ensuring that everything works, and the package only should bring the code needed to work.
But the repository is yours, so I'll fix that also 😉

@Zaharid
Copy link
Owner

Zaharid commented Mar 25, 2022

Regarding flags, these work fine

In [7]: validobj.parse_input(["RX", "READ"], Attributes)
Out[7]: <Attributes.RX: 5>

In [8]: validobj.parse_input(["RX", "WRITE"], Attributes)
Out[8]: <Attributes.RX|EXECUTE|RW|WRITE|READ: 7>

In [9]: validobj.parse_input("RX", Attributes)
Out[9]: <Attributes.RX: 5>

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.

feature request: support Enum by Value
2 participants