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.

Sign up for GitHub

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

fixed _isbool for python 3 #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fixed _isbool for python 3 #62

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cf9a94
fixed _isbool for python 3
gfrlv May 29, 2020
7a9953e
added test cases for isbool()
gfrlv Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions tabulate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,13 +610,21 @@ def _isbool(string):
"""
>>> _isbool(True)
True
>>> _isbool("False")
>>> _isbool(b"false")
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't it be b"False" rather than b"false"?

I'd rather see a different test case for b"False" and "False", than the old test being replaced with a new test.

Copy link
Contributor Author

@gfrlv gfrlv Feb 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think it should accept both b'false' and b'False', to handle data that doesn't originate from python. For example, in mycli we sometimes get booleans as strings from the SQL connector and would like to display them without parsing. But maybe that's asking too much.

Copy link
Owner

Choose a reason for hiding this comment

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

I understand where it's coming from, but it's a slippery slope. In no time we'll have to maintain all possible ways to format false and true values (.TRUE., FaLsE, nil, ...). Probably as a compromise we may decide to accept only "true" and "false", but let them be case-insensitive.

The argument for supporting only "True" and "False": these are Python literals, this is how the library already works.

The argument to supporting "True", "true", "False", and "false": it makes it easier to consume output generated by other programming languages. The argument against: it's a breaking change. The behavior of _isbool("false") -> False was not documented, but this PR will change it.

The argument to do a case-insensitive match: the same as above.

I'm very reluctant to do breaking changes to this library. Its heuristics are sort of odd, and at this point I'm pretty sure there's someone who relies on "false" being literal text. But I think your suggestion is more practical.

True
>>> _isbool(1)
False
"""
return type(string) is _bool_type or (
isinstance(string, (_binary_type, _text_type)) and string in ("True", "False")
return (
type(string) is _bool_type
or (
isinstance(string, _text_type)
and string in {"True", "False", "true", "false"}
)
or (
isinstance(string, _binary_type)
and string in {b"True", b"False", b"true", b"false"}
)
)


Expand Down