Fix: Allow validation of both JSON strings and objects #61
Conversation
There was a problem hiding this comment.
Thank you for working on this, and sorry it took me a while to get back to you. I was out of the office when the pull request came in, and I missed it.
There are a few things in the code that need to be addressed. You'll also have to sign-off on your commit before we can accept it. See the contributing guidelines for more information about how to do that and why it's required.
| # Copyright (c) 2022-2023 Intel Corporation | ||
| # SPDX-License-Identifier: MIT |
There was a problem hiding this comment.
Even though you have modified these files, the copyright notice needs to stay in place as per the license terms.
|
|
||
| def _validate_coverage_json(json_string: str) -> object: | ||
| def _validate_coverage_data(coverage_data) -> object: |
There was a problem hiding this comment.
I think it might be a good idea to be explicit that we're intending to accept any data type here:
| def _validate_coverage_data(coverage_data) -> object: | |
| def _validate_coverage_data(coverage_data: Any) -> object: |
|
|
||
| Parameters | ||
| ---------- | ||
| json_string : String | ||
| The JSON string to validate. | ||
| coverage_data : String or Object |
There was a problem hiding this comment.
| coverage_data : String or Object | |
| coverage_data : Any |
|
|
||
| TypeError | ||
| If the JSON string is not a string. | ||
| If the data is not a string or dictionary. |
There was a problem hiding this comment.
There are other types beside dict that are valid JSON, so we might not want to limit ourselves here.
| If the data is not a string or dictionary. | |
| If the data is not a string or JSON object. |
| else: | ||
| raise TypeError("Coverage must be a JSON string or dictionary.") | ||
|
|
||
| schema_path = os.path.join(os.path.dirname(__file__), "coverage-0.3.0.schema") |
There was a problem hiding this comment.
Why did you revert this instead of using pkgutil as we were before?
| def test_coverage_object_invalid(self): | ||
| """p3.data.validation.coverage_object_invalid""" | ||
| json_object = [{"file": "path", "id": "sha", "lines": [["1"]]}] | ||
| print(f"Testing invalid object: {json_object}") |
There was a problem hiding this comment.
| print(f"Testing invalid object: {json_object}") |
|
|
||
| print("Testing invalid type") |
There was a problem hiding this comment.
| print("Testing invalid type") |
| elif isinstance(coverage_data, dict): | ||
| instance = coverage_data | ||
| else: | ||
| raise TypeError("Coverage must be a JSON string or dictionary.") |
There was a problem hiding this comment.
JSON isn't necessarily a dict:
| elif isinstance(coverage_data, dict): | |
| instance = coverage_data | |
| else: | |
| raise TypeError("Coverage must be a JSON string or dictionary.") | |
| elif isinstance(coverage_data, (dict, list)): | |
| instance = coverage_data | |
| else: | |
| raise TypeError("Coverage must be a JSON string or object.") |
| """p3.data.validation.coverage_object_valid""" | ||
| json_object = [{"file": "path", "id": "sha", "lines": [1, 2, [3, 5]]}] | ||
| print(f"Testing valid object: {json_object}") | ||
| result_object = _validate_coverage_data(json_object) |
There was a problem hiding this comment.
This test currently fails. I think this is related to the comment I made above; you introduced a check that the coverage data is a dict, but here you can see that json_object is an array. I suspect the test will start passing when the comment above is addressed, but I wanted to call it out here as well.
|
|
||
| print("Testing invalid type") | ||
| with self.assertRaises(TypeError): | ||
| _validate_coverage_data(12345) # Tipo non valido |
There was a problem hiding this comment.
| _validate_coverage_data(12345) # Tipo non valido | |
| _validate_coverage_data(12345) |
|
@Chri1607 - It's been a few weeks since you opened this PR. Are you still interested in contributing? |
|
Closing as stale. @Chri1607, if you're still interested in contributing, please re-open this pull request. |
Updated the _validate_coverage_data function to handle both JSON strings and objects.
Changes:
Modified p3/data/_validation.py to validate both JSON strings and objects.
Updated imports and function calls in p3/metrics/_divergence.py.
Adjusted tests in tests/data/test_validation.py to cover both cases.