Handle x nullable for enums #191
Conversation
|
Hi Thank you |
|
I'm hesitant to add this, at least as a feature that's enabled by default. My reasoning is that IIUC Thoughts? |
|
From a user perspective I think that if I specify both x-nullable=True and enum=[x, y, z], I'm pretty explicit about null being an expected value. This does not add unexpected behaviour to an enum field, but makes the x-nullable extension apply correctly to enums (In my opinion) However: I see your point about it not being in the core spec and you are the one who will have do deal with questions from users about this behaviour. So if you still think it should be disabled by default I suggest an environment variable X_NULLABLE_ENUMS or similar. I can rewrite the PR with this change if you think that's the way to go. |
|
Alright, took a night to think it over. I'd like to meet in the middle. Your user perspective argument holds water for me, but I want a way for people who want strict validation to be able to have it (i.e. not supporting If you'll update this PR so that |
flex/validation/common.py
Outdated
| @@ -297,7 +298,7 @@ def validate_enum(value, options, **kwargs): | |||
|
|
|||
|
|
|||
| def generate_enum_validator(enum, **kwargs): | |||
| if kwargs.get('x-nullable') is True: | |||
| if not os.environ.get(FLEX_DISABLE_X_NULLABLE) and kwargs.get('x-nullable') is True: | |||
There was a problem hiding this comment.
This check should probably be if FLEX_DISABLE_X_NULLABLE not in os.environ and ... for the case where it's set to an empty value.
tests/utils.py
Outdated
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def set_env(**environ): |
There was a problem hiding this comment.
Can you update this to use the monkeypatch fixture provided by pytest. It automatically cleans up anything that you patch during test runs.
|
Good feedback :) Thanks! |
I've noticed that enums don't work very well with x-nullable=True. I had to include null/None as an enum value for the validation to pass, but that combo of x-nullable and enum value does not work with swagger-ui.
So I suggest this change.