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

Restore str representation conversion logic removed in 2.1.0 #125

Merged
merged 3 commits into from
Feb 23, 2021
Merged

Restore str representation conversion logic removed in 2.1.0 #125

merged 3 commits into from
Feb 23, 2021

Conversation

johnthagen
Copy link
Contributor

@johnthagen johnthagen commented Feb 22, 2021

Closes #124

  • Reverts part of 0eabdb4

  • Fix DeprecationWarning in test usage of ugettext_lazy

@johnthagen
Copy link
Contributor Author

@akx Would you mind taking a look? Might need a 2.2.1 to restore capability with parts of the DRF ecosystem.

@gsakkis
Copy link
Contributor

gsakkis commented Feb 22, 2021

I'm not quite sure this is a bug, or even if it is, that this is the right fix. Looking at the to_representation implementations for DRF fields, afaict none of them does validation of the input value. ChoiceField in particular returns the input value if it doesn't match any of the choices:

    def to_representation(self, value):
        if value in ('', None):
            return value
        return self.choice_strings_to_values.get(str(value), value)

In any case, a related unit test should be added; this was the reason it was removed at 0eabdb4 in the first place.

@johnthagen
Copy link
Contributor Author

In any case, a related unit test should be added; this was the reason it was removed at 0eabdb4 in the first place.

Good point. I have added several unit tests in 7164aa1.

@gsakkis
Copy link
Contributor

gsakkis commented Feb 22, 2021

FWIW to keep the same semantics with ChoiceField my fix would be:

if not isinstance(instance, self.enum):
    return instance
...
# rest left as is

I'm not a DRF expert but I don't think to_representation should be raising an exception for invalid values.

@akx
Copy link
Contributor

akx commented Feb 23, 2021

Good morning!

I think this is a good enough fix for 2.1.1 as a bug fix. We could take a look at whether to simplify things for a future version.

Merging, thanks.

EDIT: Released in 2.1.1.

@akx akx merged commit 2ce149e into hzdg:master Feb 23, 2021
@johnthagen johnthagen deleted the fix-str-representation branch February 23, 2021 12:06
@johnthagen
Copy link
Contributor Author

@akx Confirmed that 2.1.1 is working again in our production app. Thanks!

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.

2.1.0 Incompatibility with drf-yasg
3 participants