-
Notifications
You must be signed in to change notification settings - Fork 178
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
fix(modules): use parse from packaging module #14732
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.
Let's get some tests of the version handling by forcing it to evaluate some invalid versions. This was a very weird issue that happened because of packaging minutia lol
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14732 +/- ##
==========================================
+ Coverage 67.17% 67.19% +0.02%
==========================================
Files 2495 2495
Lines 71483 71403 -80
Branches 9020 8992 -28
==========================================
- Hits 48020 47982 -38
+ Misses 21341 21305 -36
+ Partials 2122 2116 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM!
@@ -16,6 +15,14 @@ | |||
TaskPayload = TypeVar("TaskPayload") | |||
|
|||
|
|||
def parse_fw_version(version: str) -> Version: |
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.
What is the difference when using parse instead of instantiating a Version object directly?
The answer is nothing, absolutely nothing.
https://github.com/pypa/packaging/blob/main/src/packaging/version.py#L47-L56
It is for backward compatibility when parse
used to return a Version or a LegacyVersion object
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.
yeah, I was mostly using it here to detect a bad version name so we can default it to 0.0.0
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.
No, your function is totally good. What I mean is all packaging.version.parse(string_version)
does is call Version(string_version)
under the hood. Ignore me lol
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.
The test looks good but it should really be in like api/tests/opentrons/hardware_control/
, not all the way over in the robot server
["v3.0.dshjfd", Version("v0.0.0")], | ||
], | ||
) | ||
async def test_catch_invalid_fw_version( |
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 should be next to the actual code it's testing
c11b186
to
bcf9f53
Compare
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.
remove the rest of the diff from the robot server and we're good, nice!
Fix for #14712.
We were using parse_version from
pkg_resources,
which can raise anInvalidVersionError
, and importingInvalidVersionError
from a separate packaging module. This imports theparse
function from the same module the error would be raised from.