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.

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

Add repr() calls to "invalid value" exception message in StrConverter.to_bool #2666

Merged
merged 6 commits into from Dec 9, 2022

Conversation

ptmcg
Copy link
Contributor

@ptmcg ptmcg commented Dec 9, 2022

After upgrading to 4.0, we are getting the following exceptions in our build pipeline:

File "/tmp/tmpw_2jv6x3/python-test-env/lib/python3.9/site-packages/tox/config/cli/parser.py", line 293, in add_color_flags
    if converter.to_bool(os.environ.get("NO_COLOR", "")):
File "/tmp/tmpw_2jv6x3/python-test-env/lib/python3.9/site-packages/tox/config/loader/str_convert.py", line 90, in to_bool
    raise TypeError(f"value {value} cannot be transformed to bool, valid: {', '.join(StrConvert.VALID_BOOL)}")
TypeError: value  cannot be transformed to bool, valid: , 0, 1, false, no, off, on, true, yes

We cannot reproduce the issue locally, else we would have more debugging tools and options.

This PR adds !r to display the repr() of the bad value, as well as repr()'s the values in VALID_BOOL, to help diagnose issues when running locally is not an option.

Thanks for contribution

Please, make sure you address all the checklists (for details on how see
development documentation)!

  • ran the linter to address style issues (tox -e fix_lint)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

src/tox/config/loader/str_convert.py Outdated Show resolved Hide resolved
@gaborbernat
Copy link
Member

Also needs a test. My guess is you're passing an empty string for boolean.

@ptmcg
Copy link
Contributor Author

ptmcg commented Dec 9, 2022

Thanks for the quick review, by the way!

That was our thought as well (passing in an empty string), but the logic in the code should be able to handle an empty string, as that appears to be one of the values in VALID_BOOL.

I looked for other tests, but did not see any for this method.

@gaborbernat
Copy link
Member

I looked for other tests, but did not see any for this method.

https://github.com/tox-dev/tox/blob/main/tests/config/test_sets.py#L92

@ptmcg
Copy link
Contributor Author

ptmcg commented Dec 9, 2022

Ah, I was looking for something specifically testing to_bool(). Found the appropriate test, updated the expected error message.

@ptmcg
Copy link
Contributor Author

ptmcg commented Dec 9, 2022

Reverted repr() on VALID_BOOL list items, and updated existing test to reflect changed message.

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.

None yet

2 participants