fix: replace strtobool for local function #128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
henriquebastos
requested changes
Dec 11, 2021
tests/test_util.py
Outdated
| def test_invalid_value_text(): | ||
| invalid_list = ["Invalid_Value_1", "1nv4l1d_V4lu3_2", "Invalid_Value_3"] | ||
| for value in invalid_list: | ||
| with pytest.raises(ValueError, match="invalid truth value '%s'".format(value)): |
There was a problem hiding this comment.
- I don't like this single test with a loop.
- The error string should be a constant without
format.
Please check pytest's parametrize
tests/test_util.py
Outdated
| @@ -0,0 +1,21 @@ | |||
| import pytest | |||
| from util import strtobool | |||
There was a problem hiding this comment.
utils is a symptom of lack of a proper abstraction. Please, do not use it. Keep the strtobool function on the main file decouple.py since it's not used anywhere else.
iurisilvio
reviewed
Dec 11, 2021
util.py
Outdated
| @@ -0,0 +1,9 @@ | |||
| def strtobool(value): | |||
| value = value.lower() | |||
| if value in ["y", "yes", "t", "true", "on", "1"]: | |||
There was a problem hiding this comment.
These comparisons are O(N) and always recreate the list. Make the list definition global and use a set instead of a list.
iurisilvio
reviewed
Jan 14, 2022
iurisilvio
reviewed
Jan 14, 2022
iurisilvio
reviewed
Jan 14, 2022
henriquebastos
approved these changes
Feb 2, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
The distutils module was deprecated in PEP 632.
resolve #127
Solution
Creation of a local function to replace the strtobool function used by distutils.