-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor validate_query() #157
Conversation
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.
Please wait for @levisingularity and @eddiebrissow approvals before merging
|
||
return True | ||
return True |
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.
I believe this should be outside the for.
return True | |
return True |
@@ -33,12 +35,45 @@ def valid_event(self, action_type): | |||
}, | |||
} | |||
} | |||
|
|||
@pytest.fixture | |||
def query_list(self, action_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.
Success are good an all for testing, but testing the invalid paths are very important as well.
If so, you would've caught the return True
inside the for.
def validate_query(queries, *args, **kwargs) -> bool: | ||
if isinstance(queries, dict): | ||
queries = [queries] | ||
elif not isinstance(queries, list) or not all(isinstance(query, dict) for query in queries): |
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 limits the query, such as [{}, []]
or [[], []]
. To ensure the parameters are correct, we can to perform a recursive check and run additional tests, or simply verify the top-level type as either a list or a dictionary.
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.
simply verify the top-level type as either a list or a dictionary.
This sounds like a good plan for now. @angeloprobst , do you have any inputs here?
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.
I think that a recursive validation like the one suggested by @Pedrobc89 in the ticket covers all scenarios.
Resolves #156