Skip to content

Added pydantic integration for Version #343

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

Conversation

calebstewart
Copy link
Contributor

This PR adds native pydantic integration to python-semver. Pydantic makes defining data schemas/models super easy and intuitive, but doesn't have a native field type for semantic version strings. Custom field types are created by simply including a class method __get_validators__ on the class which yields methods to validate input syntax and return the resulting type. In this case, the parse method functions naturally as the field validator. Pydantic also allows the __modify_schema__ method to inject extra details about the field into the model schema.

In this case, I simply added the __modify_schema__ and __get_validators__ methods to the Version class. These two class-methods are one line each. The former injects semantic versioning examples into the generated schema while the latter yields the Version.parse method as a validator. The result is that a Pydantic user can do the following:

import pydantic
import semver

class MyModel(pydantic.BaseModel):
  version: semver.Version

# model.version will be an instance of semver.Version
# and the construction will raise a pydantic.ValidationError
# if improperly formatted.
model = MyModel.parse_obj({"version": "1.2.3"})

It's worth noting that a pydantic user could manually specify Version.parse as a validator for the field in their model, but this is less expressive, and the overhead for making semver.Version compatible with pydantic is relatively low (two one-line class methods).

That being said, I totally understand if you think this is out of scope here, but it seemed like a simple quality of life improvement, and thought I'd throw it out there. I went through the contributing guidelines, and I think I've hit all the marks. Thanks for reviewing/considering! 😃

Changes:

  • Added two new dunder methods in Version which comply with the custom
    field types described here.
  • Updated usage documentation with example for using with pydantic.
  • Added test case for pydantic integration (mainly copied parsing tests, and modified to test pydantic integration).
  • Added pydantic dependency in tox.ini for tests cases.
  • Verified all tests passed for all python versions with tox.

- Added two new dunder methods in Version which comply with the custom
field types described [here](https://pydantic-docs.helpmanual.io/usage/types/#classes-with-__get_validators__).
- Updated usage documentation with example for using with Pydantic.
- Added test case for pydantic integration.
- Added pydantic dependency in `tox.ini` for tests cases.
- Verified all tests passed for all python versions with `tox`.
Copy link
Member

@tomschr tomschr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @calebstewart for your efforts and sorry for the delay! Much appreciated.

I never heard about Pydantic, but it seems to be an interesting project. Not entirely sure if this is something for this project, but I think we should give it a try. 🙂

I've added some small, minor comments. If you could have a look we could merge them.

@tomschr
Copy link
Member

tomschr commented Jan 20, 2022

After looking at this PR again and thinking about it, I'm sorry but I'm not entirely convinced we should amend the class with these methods.

Although the overall change is simple and doesn't introduce any runtime dependencies (which is good!), I have the feeling it doesn't add any value to non-Pydantic users. How many users really use semver and pydantic in this way? What should we do if another user have the same idea, but with a different library? Or what happens if Pydantic introduces some incompatible changes? Where do we draw the line? Wouldn't that lead to a "bloated" class?

I know, it's a bit of a stretch. However, my philosophy in this regard is that I try to keep any runtime dependencies away from semver. It shouldn't depend on other libraries as the overall scope is quite focused. So in other words, it should stand on its own legs. Maybe we should follow this approach with "virtual" dependencies too.

Have you tried to solve your issue by inheritance?

class PydanticVersion(Version):
   @classmethod
    def __get_validators__(cls):
        """Return a list of validator methods for pydantic models."""
        yield cls.pars

    @classmethod
    def __modify_schema__(cls, field_schema):
        """Inject/mutate the pydantic field schema in-place."""
        field_schema.update(examples=["1.0.2", "2.15.3-alpha", "21.3.15-beta+12345"])

Then you could add this to your model:

class MyModel(pydantic.BaseModel):
  version: PydanticVersion

I haven't tried it, but it looks like a small cost.

However, I don't want to completely reject it. I'd like to suggest to include it into the documentation. It could serve as an example about how we could combine semver with other projects.

Let me know if you have a compelling argument that I've overlooked. I'd really like to know! I leave this PR open for the time being.

@calebstewart
Copy link
Contributor Author

I apologize for never getting back to you on those requests. The holidays were kind of crazy, and this fell off my plate. I totally understand the apprehension here. I wasn't completely sure if it fit either, to be honest. Not a huge deal.

I think your example looks right as well. That is what I ended up doing in the end. If you'd like to add something like that as a note/example in the documentation, I think that would be really cool and possibly helpful! I have no problem with this PR being closed, though. I totally understand your position. 👍

@tomschr
Copy link
Member

tomschr commented Jan 21, 2022

All good, don't worry. I was also busy with other stuff. 😉

Thank you Caleb for your nice words! I'm really sorry, hope it wasn't too much effort on your side.

I'd proceed as follows: I'll integrate a new section into the documentation describing the above finding. @calebstewart Would you like to review it once it's finished?

tomschr added a commit to tomschr/python-semver that referenced this pull request Jan 27, 2022
@tomschr
Copy link
Member

tomschr commented Jan 27, 2022

Closing this PR in favor of #353

@tomschr tomschr closed this Jan 27, 2022
tomschr added a commit to tomschr/python-semver that referenced this pull request Jan 28, 2022
Related to issue python-semver#343

Co-authored-by: <calebstewart@users.noreply.github.com>
tomschr added a commit to tomschr/python-semver that referenced this pull request Jan 28, 2022
Related to issue python-semver#343

Co-authored-by: Caleb Stewart <calebstewart@users.noreply.github.com>
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.

2 participants