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

Pydantic 2 conversion #36

Merged
merged 18 commits into from
Aug 13, 2024
Merged

Pydantic 2 conversion #36

merged 18 commits into from
Aug 13, 2024

Conversation

ipmb
Copy link
Member

@ipmb ipmb commented Jul 18, 2023

Pydantic 2 made a lot of changes and some of them don't translate well.

@ipmb ipmb added the help wanted Extra attention is needed label Jul 18, 2023
@FabianElsmer
Copy link

Do you have a rough overview of the changes necessary to make this library compatible with pydantic 2?
I would be willing to look into it and fix the PR up as far as my abilities allow

Or is this library not being supported anymore and I should fork it either way?

@ipmb
Copy link
Member Author

ipmb commented Nov 17, 2023

There is a comprehensive test suite, so you can see the places the tests are failing with pydantic 2 and use that as your guide.

This is still supported and we'd accept a PR to resolve this.

@ipmb
Copy link
Member Author

ipmb commented Jul 10, 2024

you can drop 3.8 support

@@ -141,17 +141,17 @@ class TestConf(GoodConf):
a: bool = Field(default=False)

lines = TestConf.generate_markdown().splitlines()
assert " * type: `bool`" in lines
assert " * type: `<class 'bool'>`" in lines
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it hard to get it to maintain the original output? (no <class '...'>)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible to do!

@ipmb ipmb mentioned this pull request Jul 15, 2024
@@ -56,8 +56,7 @@ First, create a ``conf.py`` file in your project's directory, next to
description="Used for cryptographic signing. "
"https://docs.djangoproject.com/en/2.0/ref/settings/#secret-key")

class Config:
default_files = ["/etc/myproject/myproject.yaml", "myproject.yaml"]
model_config = {"default_files": ["/etc/myproject/myproject.yaml", "myproject.yaml"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

The readme still mentions Config below, should get updated to model_config as well.

@@ -208,11 +207,13 @@ def test_precedence(tmpdir):
path = tmpdir.join("myapp.json")
path.write(json.dumps({"init": "file", "env": "file", "file": "file"}))

class TestConf(GoodConf, default_files=[path]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this no longer work? The default Pydantic docs still show configuration via class arguments as supported: https://docs.pydantic.dev/2.8/api/config/

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for looking, and for the link @apollo13!
This should be updated to support configuration via class arguments

# __pydantic_private__ to {} prior to setting self._config_file.
_object_setattr(self, "__pydantic_private__", {})
self._config_file = config_file

# Emulate Pydantic behavior, load immediately
if kwargs:
return super().__init__(**kwargs)
elif load:
return self.load()
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would make sense to get rid of self.load() and always load immediately like pydantic does. We are already backwards incompatible and having .load() doesn't get you much imo. At some point it is simply not worth to fight with pydantic :) What do you think @ipmb?

@apollo13
Copy link
Contributor

apollo13 commented Jul 30, 2024

I have a patch (based on this PR) which removes the use of _pydantic internals and uses contextvars instead: apollo13@c55fa3a

The nice thing about it is that now the file source is the only one responsible for loading the data, the downside is that it uses contextvars :D

EDIT:// Updated the commit with a few fixes, better to check the branch: https://github.com/apollo13/goodconf/commits/pydantic2-fixes/

@dchukhin
Copy link
Contributor

dchukhin commented Aug 8, 2024

@apollo13 thanks for pointing to your patch. The changes seem good to me; could you update the tests based on your changes (replace the commented-out lines), and make a pull request against the main branch here? We'd like to release the changes in this pull request first, and then your improvements.

@apollo13
Copy link
Contributor

apollo13 commented Aug 8, 2024

@dchukhin I will try to find some time to do it. Have to think about the tests though. I have commented out the _config_file stuff since it is no longer used. Will have to check if we need some mocking or similar there.

@apollo13
Copy link
Contributor

apollo13 commented Aug 9, 2024

@dchukhin What do you mean with "and make a pull request against the main branch here?". Should I include your changes in that PR? Or should we wait till this PR is merged and then I can make one against main with just the delta.

@dchukhin
Copy link
Contributor

@apollo13 Ah yes, sorry for the confusion. We'll be releasing this work soon, so once that's done, you should be able to make a pull request against main with your changes

@dchukhin dchukhin merged commit 2aa3334 into main Aug 13, 2024
8 checks passed
@dchukhin dchukhin deleted the pydantic2 branch August 13, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants