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: make pydantic errors (un)pickable #1630

Merged

Conversation

PrettyWood
Copy link
Member

Change Summary

Custom pydantic errors (subclasses of PydanticTypeError and PydanticValueError) were not (un)pickable. Now they are!

Related issue number

closes #1616

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@PrettyWood PrettyWood force-pushed the fix/pickable-validation-error branch from 8a82658 to 81f58d7 Compare June 13, 2020 22:24
@codecov
Copy link

codecov bot commented Jun 13, 2020

Codecov Report

Merging #1630 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1630   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          21       21           
  Lines        3799     3803    +4     
  Branches      756      756           
=======================================
+ Hits         3797     3801    +4     
  Misses          1        1           
  Partials        1        1           
Impacted Files Coverage Δ
pydantic/errors.py 100.00% <100.00%> (ø)
pydantic/class_validators.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a2d787...9a25054. Read the comment docs.

@@ -101,6 +112,9 @@ def __init__(self, **ctx: Any) -> None:
def __str__(self) -> str:
return self.msg_template.format(**self.__dict__)

def __reduce__(self): # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

@samuelcolvin I thought ignoring typing made more sense for this one as it made the code more complicated for nothing.
But if you want me to, I can add this commit PrettyWood@753abdd

Copy link
Member

Choose a reason for hiding this comment

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

I think we should include type hints if at all possible

@abadger
Copy link

abadger commented Jun 25, 2020

I have verified that this PR fixes the bug I reported in #1616 Thanks @PrettyWood !

@abadger
Copy link

abadger commented Jun 25, 2020

Oh, for the record.... I did not test the Cython version of pydantic with this PR applied. I couldn't figure out how that would be installed locally.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise I think this looks good.

@@ -91,6 +91,17 @@
)


def cls_kwargs(cls, kwargs): # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def cls_kwargs(cls, kwargs): # type: ignore
def cls_kwargs(cls: Type[T], kwargs: Dict[str, Any) -> T: # type: ignore

Or something?

Even if mypy can't handle it should still add type hints.

Copy link
Member

Choose a reason for hiding this comment

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

also, shouldn't this be cls_kwargs(cls, ctx)?

Copy link
Member Author

Choose a reason for hiding this comment

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

You would like this method to be more generic with Type[T] and ctx for args and kwargs ? Sorry if I misunderstood

Copy link
Member

Choose a reason for hiding this comment

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

I mean:

  1. we should have type annotations
  2. i think it would make more sense to name the second argument ctx

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright ! I added the annotations that I already linked and renamed kwargs into ctx.

@@ -101,6 +112,9 @@ def __init__(self, **ctx: Any) -> None:
def __str__(self) -> str:
return self.msg_template.format(**self.__dict__)

def __reduce__(self): # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

I think we should include type hints if at all possible

@samuelcolvin samuelcolvin merged commit e5fff9c into pydantic:master Jun 27, 2020
@PrettyWood PrettyWood deleted the fix/pickable-validation-error branch June 27, 2020 18:37
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.

Make ValidationError unpicklable
3 participants