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

String to boolean conversion #9

Closed
LukeMathWalker opened this issue Jan 7, 2019 · 5 comments
Closed

String to boolean conversion #9

LukeMathWalker opened this issue Jan 7, 2019 · 5 comments

Comments

@LukeMathWalker
Copy link

Hi!
We had a confusing encounter today while using type_checked_constructor with convert=True.
A minimal reproducible example is the following:

from typing import NamedTuple, Optional
from undictify import type_checked_constructor

@type_checked_constructor(convert=True)
class Example(NamedTuple):
    option = Optional[bool]

if __name__ == "__main__":
   example = Example(option="False")
   assert example.option is True

This is due to Python casting any string to True if it's not empty, but it comes out as surprising if, as in our case, you are parsing a dictionary of GET parameters, that come in as strings.
What would you suggest as the best course of action? Is this something you might want to address in undictify or should we build a thin conversion layer just for booleans before feeding data into a class decorated with undictify's type_checked_constructor?

@Dobiasd
Copy link
Owner

Dobiasd commented Jan 7, 2019

Usually I'd say the default behaviour of Python might be the least surprising to a user of a library. But in that particular case it would mean that converting 'False' to True is the "sane" thing to do. But this does not sound very sane to me. 🙂

So I guess it might make sense to handle this in undictify. What strings should we convert to True and False? Maybe like so?

  • True <- ['1', 'yes', 'true', 'on']
  • False <- ['0', 'no', 'false', 'off']
  • exception for everything else?

(With the string being converted to lowercase first?)

@Dobiasd Dobiasd closed this as completed in ac52ea8 Jan 7, 2019
@Dobiasd
Copy link
Owner

Dobiasd commented Jan 7, 2019

It just makes so much sense, so I quickly implemented it. :)

@Dobiasd
Copy link
Owner

Dobiasd commented Jan 7, 2019

Thanks again a lot for this very helpful report. Stuff like this really improves the library. :)

@LukeMathWalker
Copy link
Author

Wow, I wasn't even able to answer - thanks so much for the quick response!
I don't about on/off or yes/no, but 0/1 and false/true for sure make sense :)

@Dobiasd
Copy link
Owner

Dobiasd commented Jan 7, 2019

If a component in the the standard library does it that way, I guess it's fine. :)

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

2 participants