Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

pmeier
Copy link
Contributor

@pmeier pmeier commented May 13, 2024

Fixes #1296.

@AyushExel
Copy link
Contributor

Thankyou!

Copy link
Contributor

@wjones127 wjones127 left a 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?

@wjones127
Copy link
Contributor

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__)
Copy link
Contributor

@wjones127 wjones127 May 16, 2024

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.)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@pmeier
Copy link
Contributor Author

pmeier commented May 16, 2024

Superseded by #1311.

@pmeier pmeier closed this May 16, 2024
@pmeier pmeier deleted the deprecation-warnings branch May 16, 2024 19:50
wjones127 added a commit that referenced this pull request May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix semvar deprication warning
3 participants