Chore/add unit tests #52
Conversation
There was a problem hiding this comment.
all remarks and suggestions can be marked resolved without implementing, they are not needed for approval but encouraged if you find the alternative design appealing.
Holding off on approving until warning and questions comments are addressed, as well as the cycle function. I (or someone else) will need more time to review that.
|
|
||
| script: | ||
| - poetry run pytest -vv | ||
| - poetry run pytest -vv tests |
There was a problem hiding this comment.
warning: this looks for a tests directory right? If so, I don't see a tests directory.
| #################### | ||
|
|
||
|
|
||
| parser = argparse.ArgumentParser(description='Validate JSON') |
There was a problem hiding this comment.
suggestion: since parser is only used for args, you can move this to a function call to simplify the main flow
|
|
||
| for f in args.jsonfiles: | ||
| doc = json.load(f) | ||
| if args.invalid: |
There was a problem hiding this comment.
suggestion: this should be moved outside the function since args is/seems to be unmodified. Even further, you can move the contents to each block to a function and assign that function to a variable.
e.g.
def check_is_invalid(f, doc):
# ...
def check_is_valid(f, doc):
# ...
check_validation = check_is_invalid if args.invalid else check_is_valid
for json_file in args.jsonfiles:
doc = json.load(f)
check_validation(f, doc)| try: | ||
| print("CHECK if {0} is invalid:".format(f.name), end=" ") | ||
| print(type(doc)) | ||
| if type(doc) == dict: |
There was a problem hiding this comment.
suggestion: this if-elif-else section matches the same section except for the else case and can thus be abstracted provided you pass in a custom message
e.g.
def handle_validation(doc, dictionary, unknown_type_handler):
if type(doc) == dict:
validate_entity(doc, dictionary.schema)
elif type(doc) == list:
for entity in doc:
validate_entity(entity, dictionary.schema)
else:
unknown_type_handler()There was a problem hiding this comment.
there are better ways to do it than what I've written here if I stared at the code longer, but in terms of simplicity I thought of this first.
| validate_entity(entity, dictionary.schema) | ||
| else: | ||
| raise ValidationError("Invalid json") | ||
| except ValidationError as e: |
There was a problem hiding this comment.
suggestion: I don't think this exception has much use can can be removed to simplify behavior since you're just printing
| If the acyclicality check passes like this, either the algorithm is | ||
| missing something or we're ignoring types that we shouldn't. | ||
| """ | ||
| # Add this test if any cyclic schemas are added, eg, 'file' and 'archive' |
There was a problem hiding this comment.
warning: this function has no body. That may cause issues with python's tab-based syntax? I'll check this out on my local branch to see. Either way, I would suggest either removing the function or asserting not implemented
| """ | ||
| path = path if path is not None else [] | ||
| for key in b: |
There was a problem hiding this comment.
suggestion: types could be helpful for the params. are a and b sets? If so, doing set comparison can be simpler and faster. set(a & b), for example, can give you a collection such that if key in a will be true, and you can do different to find the keys of a not in b
| for s in schemata.values(): | ||
| validate(s, metaschema) | ||
|
|
||
| def assert_link_is_also_prop(link): |
There was a problem hiding this comment.
suggestion: move def outside the for loop. you can use partial to define it with the s in context
| assert_link_is_also_prop(link) | ||
|
|
||
|
|
||
| def check_for_cycles(schemata, ignored_types=None): |
There was a problem hiding this comment.
remark: this is an interesting problem that is giving me leetcode flashbacks, I'll come back to this with some thoughts later if I have any.
Jira Tickets: PXP-11414, PXP-11243, PPS-710
New Features
Breaking Changes
Bug Fixes
Improvements
testsdirectoryDependency updates
Deployment changes