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

feat: migrate to pydantic v2 #51

Closed

Conversation

chuckliu1979
Copy link

bpx still use pydantic v1, our project need pydantic v2, hence i migrate this project to pydantic v2

@chuckliu1979
Copy link
Author

rollback typing hint for python 3.8 compat, tested on python 3.8/3.9/3.10/3.11/3.12, all pass.

@agriyakhetarpal
Copy link
Collaborator

Apologies for seeing this late, @chuckliu1979 – just triggered CI for you here, thanks!

from .parsers import parse_bpx_str, parse_bpx_obj, parse_bpx_file
from .utilities import get_electrode_stoichiometries, get_electrode_concentrations

def parse_bpx_obj(bpx: dict, v_tol: float = 0.001) -> BPX:
Copy link
Collaborator

Choose a reason for hiding this comment

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

why have these been moved here from parsers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, this should go back to parsers

Copy link
Collaborator

Choose a reason for hiding this comment

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

I covered this in #79

@rtimms
Copy link
Collaborator

rtimms commented Dec 16, 2024

@agriyakhetarpal do you think you could help with a review? The changes to pydantic look ok to me but there are some other changes to tests etc. it would be good to get a second pair of eyes on

@rtimms
Copy link
Collaborator

rtimms commented Dec 16, 2024

also note we dropped python 3.8

@agriyakhetarpal
Copy link
Collaborator

Yes, thanks for the ping – I'll try to review this week!

@agriyakhetarpal agriyakhetarpal self-requested a review December 16, 2024 15:42
from .parsers import parse_bpx_str, parse_bpx_obj, parse_bpx_file
from .utilities import get_electrode_stoichiometries, get_electrode_concentrations

def parse_bpx_obj(bpx: dict, v_tol: float = 0.001) -> BPX:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, this should go back to parsers

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this removed during a PyDantic change? Can we split that into a different PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary? I don't see how this is related

Comment on lines +13 to +16
if TYPE_CHECKING:
from collections.abc import Callable

from pydantic import GetCoreSchemaHandler, GetJsonSchemaHandler
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just enable type checking all the time

readme = "README.md"
dynamic = ["version", "description"]
requires-python = ">=3.8"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably should not be 3.8 anymore

Copy link
Collaborator

Choose a reason for hiding this comment

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

I covered this in #79

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we need to rewrite this whole file?

@kratman kratman mentioned this pull request Dec 16, 2024
@rtimms rtimms closed this Dec 17, 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.

4 participants