Enforce is_valid(raise_exception=False) as a keyword-only argument. by ProstoMaxim · Pull Request #7952 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation6 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

ProstoMaxim

At the moment is_valid calls are possible without keyword-only arguments, so code
serializer.is_valid(True) is possible and will be quite misleading, especially for new developers. This pull requests makes "raise_exception" name mandatory if serializer is called with argument.

@ProstoMaxim

@ProstoMaxim

@stale

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tomchristie tomchristie changed the titlePrevent misleading is_valid calls Enforce is_valid(raise_exception=False) as a keyword-only argument.

Aug 10, 2022

@tomchristie

This seems like it could be a pretty reasonable change to me, along with a major version bump.

Granted there's probably some codebases that might be forced into a change, OTOH, it is an argument that makes much more sense as a keyword-only case.

Any other maintainers have an opinion on the trade-off here?

@adamchainz

I like it! This avoids a problem known as "the boolean trap".

@romanek-adam-b2c2

It like it too, but it breaks the existing API and changelog doesn't mention that :(

@tomchristie

@romanek-adam-b2c2

OK, then it would make sense to include a link to the former in the latter, as originally I only found and read the latter (as I was reviewing tons of changelogs for multiple packages which got updated for my project). Additionally I'd suggest explicitly marking breaking changes with some prefix / tag / whatever ("BREAKING: ...") to make sure no one will miss that. Anyway, keep up the good work!

sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request

Dec 3, 2022

@ProstoMaxim @sigvef

…encode#7952)