Skip to content

Fix false boolean configuration values - #312

Open
bensynapse wants to merge 2 commits into
laurentS:masterfrom
bensynapse:fix-false-boolean-config
Open

bensynapse wants to merge 2 commits into
laurentS:masterfrom
bensynapse:fix-false-boolean-config

Conversation

@bensynapse

Copy link
Copy Markdown

I run Live Tennis API.

A False default now uses Starlette's boolean parser when loading configuration. Previously, false remained a nonempty string and enabled optional behavior.

The tests cover environment variables and files, constructor defaults, invalid values, and HTTP header and storage failure behavior.

poetry run coverage run --omit="tests*" -m pytest passes all 132 tests on Python 3.9, 3.10 and 3.11. Black, mypy, flake8 --select F401 and coverage reporting also pass on all three versions. Twenty-one regression cases fail before the fix.

Fixes #311.

A false default must still select Starlette's boolean parser. Otherwise
configuration strings such as false enable headers and optional storage
error handling. Keep the existing path for other defaults.
@agustin18

Copy link
Copy Markdown

Nice catch! Just wondering, maybe if default_value is not None: instead of if default_value: is simpler and cleaner here?

Because right now if someone passes default_value=0, it still skips casting since 0 is falsy, so it returns a string instead of an int.
With is not None, type(False) is already bool so Starlette parses the boolean, and it also handles 0 and other falsy defaults without needing the extra if default_value is False check.

@bensynapse

Copy link
Copy Markdown
Author

Fixed in 93c53be. The condition now uses is not None, so zero defaults get their numeric type and False uses Starlette's boolean parser.
I removed the separate False branch and added ten cases for typed values and missing settings. All 142 tests pass on Python 3.9 to 3.11.
Black, mypy and the CI unused import check pass.

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.

False boolean configuration values are treated as enabled

2 participants