Rm strtobool #1964
Rm strtobool #1964
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Oh wow, nice catch, I didn't know that strtobool was deprecated. There doesn't seem to be any warning when importing it, who knows how many upgrades will fail when people switch to Python 3.12.
I like the decision to change the name, that way there can be no accidental usage of the previous function.
I only have one question, which is about the decision to return a bool instead of int now. I agree it makes more sense (maybe strtobool predates the existence of bools in Python), but it makes all the if str_to_bool(...) == 0 calls a bit awkward.
| @@ -28,7 +42,7 @@ def get_int_from_env(env_keys, default): | |||
| def parse_flag_from_env(key, default=False): | |||
| """Returns truthy value for `key` from the env if available else the default.""" | |||
| value = os.environ.get(key, str(default)) | |||
| return strtobool(value) == 1 # As its name indicates `strtobool` actually returns an int... | |||
| return str_to_bool(value) == 1 # As its name indicates `str_to_bool` actually returns an int... | |||
There was a problem hiding this comment.
This comment, which I assume is meant ironically, could be removed if we actually return a bool. But technically, a bool is an int, so not sure if it's really meant to be ironic :D
What does this PR do?
As
strtoboolwill be removed in the next python version (3.12), this PR re-implements it for what we care aboutTags along with #1963 to get us to ~.9s for maximum efficiency too 🚀
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@BenjaminBossan