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

Make bool_validator strict #617

Merged
merged 15 commits into from Aug 10, 2019
Merged

Make bool_validator strict #617

merged 15 commits into from Aug 10, 2019

Conversation

dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Jun 23, 2019

Change Summary

Make bool_validator a little more strict, in line with #579

In particular, allow True, False, 1, 0, all yaml true/false strings, the strings 't'/'T'/'f'/'F' (following DRF), and the bytes versions of those strings. Anything else raises a ValidationError.

Also adds RelaxedBool to retain current functionality if desired.

Related issue number

#579


Reviewing the discussion in #579, it wasn't clear to me whether the goal was to modify StrictBool to allow these strings, or to change bool_validator. My current interpretation is that the goal was to change bool_validator to bring it in line with more expected/useful behavior in a non-python context. If that's wrong, I'd be happy to refactor so that this logic applies to StrictBool instead.

This would be a breaking change for a very common type (if modifying bool_validator anyway), so I wouldn't necessarily expect it to be merged prior to a 1.0 release, but figured I'd start the process.


Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>

pydantic/validators.py Outdated Show resolved Hide resolved
@codecov
Copy link

@codecov codecov bot commented Jun 23, 2019

Codecov Report

Merging #617 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #617   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2710   2718    +8     
  Branches      532    534    +2     
=====================================
+ Hits         2710   2718    +8

tests/test_types.py Show resolved Hide resolved
Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Otherwise I'm happy with this.

@cazgp, you created the issue. Feedback here welcome.

pydantic/validators.py Outdated Show resolved Hide resolved
pydantic/validators.py Outdated Show resolved Hide resolved
@cazgp
Copy link
Contributor

@cazgp cazgp commented Jun 26, 2019

Thanks for this!

tests/test_types.py Show resolved Hide resolved
@dmontagu
Copy link
Collaborator Author

@dmontagu dmontagu commented Jun 28, 2019

@samuelcolvin I've updated the docs. However, I still have two questions:

  1. should I update the history? I'm not sure if the goal is to include this in an upcoming 0.x release or wait for 1.0
  2. I only made note of the change in the section of the docs about StrictBool and RelaxedBool. I think this is okay, but given this is a breaking change to the behavior of bool, should a note be placed somewhere else as well?

@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Jun 28, 2019

This pull request introduces 1 alert when merging fc41a25 into ccdf8e1 - view on LGTM.com

new alerts:

  • 1 for Module-level cyclic import

@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Jun 28, 2019

This pull request introduces 1 alert when merging 1a73534 into ccdf8e1 - view on LGTM.com

new alerts:

  • 1 for Module-level cyclic import

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Looking pretty good, RelaxedBool seems good.

See #576 (comment) for discussion on how we deal with history/releases.

docs/index.rst Outdated

If you want more permissive behavior, ``RelaxedBool`` can be used to cast values to bool using standard python logic.
For example, ``None`` would be cast to False. Note that this behaves differently than the standard ``bool`` field
for some values: for example, a ``RelaxedBool`` field would cast the string "false" to True.
Copy link
Owner

@samuelcolvin samuelcolvin Jun 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for some values: for example, a ``RelaxedBool`` field would cast the string "false" to True.
for some values: for example, a ``RelaxedBool`` field would cast the string ``"false"`` to ``True``.

docs/index.rst Show resolved Hide resolved
pydantic/validators.py Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
@samuelcolvin samuelcolvin mentioned this pull request Jun 28, 2019
5 tasks
@dmontagu dmontagu force-pushed the strictbool branch 3 times, most recently from 7dc18d0 to e0bfe47 Compare Jul 16, 2019
@dmontagu dmontagu force-pushed the strictbool branch 2 times, most recently from 21c28d8 to 773e160 Compare Jul 25, 2019
@samuelcolvin samuelcolvin added this to the Version 1 milestone Aug 5, 2019
@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Aug 6, 2019

please can you rebase and move the history change to changes/ as per #719.

HISTORY.rst Outdated Show resolved Hide resolved
docs/examples/exotic.py Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
pydantic/errors.py Outdated Show resolved Hide resolved
@@ -243,6 +244,21 @@ def validate(cls, value: str) -> str:
return validate_email(value)[1]


class RelaxedBool(int):
Copy link
Owner

@samuelcolvin samuelcolvin Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

humm, I'm not sure Relaxed is a good choice of word.

I personally think this type is very wierd and people shouldn't use it since it makes absolutely zero sense to parse 'false' to False.

I think either we should:

  • rename it to something like StandardLibBool and recommend against using it
  • or, (better in my opinion) remove it altogether. Please can implement it themselves if they really want it.

Copy link
Collaborator Author

@dmontagu dmontagu Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only reason to use it would be performance; right now if you want speed when you know you are getting the right type, it's awkward. But I agree it would be better if the name reflected this.

Actually I guess the default validator is pretty fast if it is already the right type?

I'd be okay either dropping or giving a different name. (I'll keep my commits locally for when someone asks for it 😈)

Copy link
Owner

@samuelcolvin samuelcolvin Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, let's keep it but say that in the docs.

Copy link
Collaborator Author

@dmontagu dmontagu Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samuelcolvin I did some minor investigation of the timing and I think the speed difference between the two bool validators is negligible (when the inputs are bools); in my runs I didn't see a clear winner.

I think probably the only real world use case where it might really be helpful is if you wanted to accept a custom type as a field input and had a custom __bool__ implementation.

At any rate, based on the above I can't think of anything worth saying in the docs except that it follows built-in python logic for boolean casting (what they currently say). Let me know if you still disagree, or if this changes your mind back to wanting to remove RelaxedBool.

Copy link
Owner

@samuelcolvin samuelcolvin Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say let's remove it then. Nothing to stop people implementing it themselves if they really want it. But we we think there's basically no context in which you should use it, it shouldn't be added.

Sorry to flip flop.

pydantic/validators.py Show resolved Hide resolved
tests/test_types.py Show resolved Hide resolved
David Montague and others added 4 commits Aug 8, 2019
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
@samuelcolvin samuelcolvin merged commit 72edca7 into samuelcolvin:master Aug 10, 2019
10 checks passed
@samuelcolvin samuelcolvin mentioned this pull request Aug 12, 2019
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.

None yet

3 participants