-
Notifications
You must be signed in to change notification settings - Fork 211
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 semver deprecation warnings #1298
Conversation
Thankyou! |
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 seems to be reverting the changes from #1253 where we were trying to expand the compatibility of semver
dependency. Does this method support older versions of semver
?
cc @AmanKishore |
@@ -37,7 +37,7 @@ | |||
import pydantic | |||
import semver | |||
|
|||
PYDANTIC_VERSION = semver.parse_version_info(pydantic.__version__) | |||
PYDANTIC_VERSION = semver.Version.parse(pydantic.__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.
Yeah this function was introduced in semver
version 3.0.0, so using this breaks compatibility with 2.x versions of semver
.
One alternative I've suggested is using packaging.version.parse()
from packaging. This is a more common dependency than semvar
, so it's likely users already have it downloaded, and the parse function there has been around since nearly the earliest version of it and supports parsing semver versions. (I just confirmed that since 14.1.0 of packaging
it handles semvar versions correctly.)
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.
Happy to change the PR to depend on packaging instead. And I agree, that is indeed the much more common dependency.
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.
It might be a little more work, since the version object returned isn't quite the same. But that seems like the best solution to me.
Superseded by #1311. |
Fixes #1296 per title. See #1298 (comment) Cc @wjones127 --------- Co-authored-by: Will Jones <willjones127@gmail.com>
Fixes #1296.