Overflow and Type Errors added to concept.py . Required tests for qua… #77
Conversation
…ntity class added in tests.
There was a problem hiding this comment.
Hey @Vortex-21 , this is really great! I think the function is doing exactly the right thing and I think the test cases have great coverage. I've left a few comments on some minor issues. Most of these will be resolved by making sure to run steps 3 and 6 in the contribution guide.
.python-version
Outdated
| @@ -0,0 +1 @@ | |||
| 3.10.11 | |||
There was a problem hiding this comment.
This file is typically used for local Python version management and shouldn't be tracked in the repository. Could you please unstage this file and add it to your .gitignore? Thanks!
There was a problem hiding this comment.
Unstaged ".python-version" and added to .gitignore .
pyproject.toml
Outdated
| @@ -35,6 +35,7 @@ termcolor = "^2.4.0" | |||
| spyne = "^2.14.0" | |||
| lxml = "^5.2.2" | |||
| xmltodict = "^0.13.0" | |||
| pytest = "^8.3.3" | |||
There was a problem hiding this comment.
Pytest is already in the dev dependencies so doesn't need to be added to the pyproject.toml. (If you check the contribution guide in the repo it has instructions for installing dev dependencies). Can you remove this line and regenerate the poetry.lock?
There was a problem hiding this comment.
Also, by installing the dev dependencies you'll get pre-commit and ruff installed. These are really neat tools that will check your code for some common style errors (number of new lines between functions, max line length) and auto-correct them when you run git commit.
There was a problem hiding this comment.
poetry.lock regenerated and dev dependencies installed !
tests/test_quantity_class.py
Outdated
| exception_info_str = str(exception_info.value) | ||
| assert any(msg in exception_info_str for msg in ["CANNOT", "Invalid value"]) | ||
|
|
||
| # if __name__ == '__main__': |
There was a problem hiding this comment.
Can you remove the commented out lines?
There was a problem hiding this comment.
Code formatted and commented out lines removed !
Good to hear that the function works fine! Apologies for the delay . I have made the necessary changes . |
| @field_validator("value") | ||
| @classmethod | ||
| def validate_value(cls, value: Union[str, float]): | ||
| if value is None: |
There was a problem hiding this comment.
I ran the testing suite and got some errors from this line. The logic here should be slightly different. If None is passed in then we want to just return None back, however if a type other than str or float is passed then we can raise the warning. I've added a suggestion below (written on my phone so will need some edits).
| if value is None: | |
| if value is None: | |
| return None | |
| elif type(value) is not str or float: | |
| raise TypeError |
There was a problem hiding this comment.
Okay got it ! Will make the change.
|
I have updated the code and removed validation for None . Updated the tests accordingly as well. |
|
Looks like in the tests/test_cdaannotator.py in line 71 and 76 |
|
Yes these tests will need to be updated too. You can check them locally with "poetry run pytest". |
|
tests updated ! |
Description
Added two more valiation for TypeError(specifically for NoneType objects.) and Overflow Error ( rare ) . Also added a set of tests in tests/test_quantity_class.py to check the validity of the code .
Related Issue
#68
Changes Made
Testing
Tested upon valid inputs (valid strings , integers and floats), invalid inputs (invalid strings) and edge cases (empty strings, None Values).
Checklist
Additional Notes