Skip to content

fixed _isbool for python 3#62

Closed
gfrlv wants to merge 2 commits intoastanin:masterfrom
gfrlv:fix_isbool
Closed

fixed _isbool for python 3#62
gfrlv wants to merge 2 commits intoastanin:masterfrom
gfrlv:fix_isbool

Conversation

@gfrlv
Copy link
Copy Markdown
Contributor

@gfrlv gfrlv commented May 29, 2020

_isbool works incorrectly with binary data in Python 3:

b'True' in ('True', 'False') == False

tabulate.py Outdated
>>> _isbool(True)
True
>>> _isbool("False")
>>> _isbool(b"false")
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@gfrlv gfrlv Feb 17, 2021

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
Copy Markdown
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.

@astanin astanin added this to the v0.9 milestone Feb 17, 2021
@astanin astanin removed this from the v0.9 milestone Oct 19, 2022
@gfrlv gfrlv closed this by deleting the head repository Jul 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants