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

Boolean type should fallback to False instead of True!!! #250

Open
daveoncode opened this issue Jan 21, 2016 · 6 comments
Open

Boolean type should fallback to False instead of True!!! #250

daveoncode opened this issue Jan 21, 2016 · 6 comments

Comments

@daveoncode
Copy link

If I deserialize a dictionary containing None as value for a boolean node, Colander falls back to True, which is absolutely wrong... a default for a boolean must always be False by definition.
I currently subclassed Boolean in this way to make it work as expected:

class SmartBoolean(Boolean):
    def deserialize(self, node, cstruct):
        if cstruct in (null, None, ''):
            return null
        return super().deserialize(node, cstruct)

But I hope you will consider to replace True with False in the last line of original Boolean deserialize() method. Thanks!

@digitalresistor
Copy link
Member

Please create a PR with tests and sign CONTRIBUTORS.txt if you haven't, and we can pull it into colander!

@mmerickel
Copy link
Member

a default for a boolean must always be False by definition

Not to be too pedantic but I don't think that's actually in the definition of a boolean. I can't find anything about what the default should be on wikipedia. Most values actually default to True exception 0, None and a few empty collections in python.

ANYWAY I'm not against changing this, but someone who cares (you) will need to submit a PR with tests as Bret said. :-)

@tisdall
Copy link
Contributor

tisdall commented Jan 22, 2016

Looking at the source for Boolean it looks like you can define how you want to serialize/deserialize values. It says:

The behaviour for values not contained in :attr:`false_choices`
depends on :attr:`true_choices`: if it's empty, any value is considered
``True``; otherwise, only values contained in :attr:`true_choices`
are considered ``True``, and an Invalid exception would be raised
for values outside of both :attr:`false_choices` and :attr:`true_choices`.

So, because the default for false values is 'false' or 0, None ends up being considered True.

Like most of colander, it's probably a good idea to translate None values from your database into colander.null and then you'll get true, false, and null values.

@daveoncode
Copy link
Author

I will submit a PR soon...
in the meanwhile, just google "default boolen value" or try this in a python shell:
assert bool() is False
then...
assert bool() is True
boom! :D

@digitalresistor
Copy link
Member

@daveoncode why not add str(None) as a value to false_choices?

For example:

class Boolean(colander.Boolean):
    """Overrides the default false_choices to include 'None' unless false_choices is provided."""

    def __init__(self, **kwargs):
        kwargs["false_choices"] = kwargs.get("false_choices", ('false', '0', str(None)))
        super().__init__(
            **kwargs,
        )

In the code sample you supplied in
#250 (comment) you return colander.null which may do what you expect it to do only if you set missing=False on your SchemaNode, so it will still not return False by default.

@twillis
Copy link

twillis commented Jun 22, 2022

this is very surprising behavior that just bit me in the ass in production no less.

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

No branches or pull requests

5 participants