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

Fix configobj to relay tuple type errors #1633

Merged
merged 5 commits into from
Nov 27, 2023

Conversation

brandonpayton
Copy link
Contributor

Some current config tuple checking is of the form:

if not isinstance(value[1], int) or not isinstance(value[2], int):
    VdtTypeError(value)

This has two problems. The first is that isinstance('123', int) is always False, which means the if's block always runs. A VdtTypeError would always be raised except that the raise keyword is missing.

This PR attempts to fix that issue.

@lucc
Copy link
Collaborator

lucc commented Nov 26, 2023

This width_tuple function looks quite simple. Do you mind adding some tests to tests/utils/test_configobj.py? Preferably also for the error handling that you improved (i.e. asserting that the right exceptions are thrown).

@brandonpayton
Copy link
Contributor Author

I committed some tests but need to break them into smaller pieces and add one or two more.

@brandonpayton
Copy link
Contributor Author

Thanks also for taking a look at this.

@brandonpayton
Copy link
Contributor Author

brandonpayton commented Nov 27, 2023

OK, I added some width_tuple tests, made the raised exceptions more precise, and consolidated the exception handling. This should be ready for another look.

Copy link
Collaborator

@lucc lucc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself looks good. I suggest breaking the test cases up into individual functions so that each function checks exactly "one thing".

tests/utils/test_configobj.py Show resolved Hide resolved
tests/utils/test_configobj.py Outdated Show resolved Hide resolved
tests/utils/test_configobj.py Show resolved Hide resolved
tests/utils/test_configobj.py Show resolved Hide resolved
tests/utils/test_configobj.py Outdated Show resolved Hide resolved
tests/utils/test_configobj.py Show resolved Hide resolved
tests/utils/test_configobj.py Show resolved Hide resolved
Co-authored-by: Lucas Hoffmann <lucc@users.noreply.github.com>
@brandonpayton
Copy link
Contributor Author

That is much cleaner. Thank you!

@lucc lucc merged commit e022585 into pazz:master Nov 27, 2023
2 checks passed
@lucc
Copy link
Collaborator

lucc commented Nov 27, 2023

Thank you for your contribution 👍

@brandonpayton brandonpayton deleted the fix-config-type-error-reporting branch November 27, 2023 19:14
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