-
Notifications
You must be signed in to change notification settings - Fork 885
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
raise error for wrong input in ignore_variables #826
Conversation
Codecov Report
@@ Coverage Diff @@
## master #826 +/- ##
==========================================
+ Coverage 98.15% 98.15% +<.01%
==========================================
Files 121 121
Lines 10920 10939 +19
==========================================
+ Hits 10718 10737 +19
Misses 202 202
Continue to review full report at Codecov.
|
@@ -165,6 +165,9 @@ def __init__(self, | |||
|
|||
self.ignore_variables = defaultdict(set) | |||
if ignore_variables is not None: | |||
# check if ignore_variables is not {str: list} | |||
if not all(isinstance(i, str) for i in ignore_variables.keys()) or not all(isinstance(i, list) for i in ignore_variables.values()): | |||
raise TypeError('ignore_variables should be dict[str -> list[str]]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test case that triggers this error?
@@ -162,6 +162,18 @@ def test_ignores_variables(es): | |||
assert 'value' not in variables | |||
|
|||
|
|||
@pytest.fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a pytest fixture. Fixtures are functions that get used by the test cases. For example, this test case uses the es
fixture, which returns an entityset.
Now it would be a good time to know if I should add the third check to see if the list is made of exclusively strings, and not say, ints or floats. |
That would be good to check as well. I think only the new "fixes" changelog entry is necessary. The "testing changes" section is for PRs that only change the tests. |
we should also test for the case where not all keys are string type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Pull Request Description
As mentioned in #818, ignore_variables does not raise error when given a wrong input.
Currently it checks if the input is
{str: list}
only, would like to know if its needed to check the values of the list to be string or not.After creating the pull request: in order to pass the changelog_updated check you will need to update the "Future Release" section of
docs/source/changelog.rst
to include this pull request.