E52F Support assert statements inside validators by abdusco · Pull Request #653 · pydantic/pydantic · GitHub
[go: up one dir, main page]
Skip to content

Support assert statements inside validators #653

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.

Sign up for GitHub

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

Jump to bottom
Merged
samuelcolvin merged 13 commits into from
Aug 15, 2019
Merged

Support assert statements inside validators #653

samuelcolvin merged 13 commits into from
Aug 15, 2019

Conversation

@abdusco
Copy link
Contributor
@abdusco abdusco commented Jul 11, 2019 •
edited by dmontagu
Loading

Change Summary

This PR adds support for assert statements inside validators (related: #654). This means we can use:

class CreateJob(BaseModel):
    name: str
    interval: int

    @validator('interval')
    def parse_interval(cls, v: int):
        assert v > 60, 'Interval must be longer than 60 seconds'
        return timedelta(seconds=v)

to raise validation errors.

I've updated docs, but haven't been able to build it on Windows. I'm getting this error:

(venv) D:\Development\oss\pydantic>make docs
make -C docs html
make[1]: Entering directory 'D:/Development/oss/pydantic/docs'
rm -rf _build
./schema_mapping.py
env: 'python3': Permission denied
make[1]: *** [Makefile:17: html] Error 126
make[1]: Leaving directory 'D:/Development/oss/pydantic/docs'
make: *** [Makefile:97: docs] Error 2

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>

@codecov
Copy link
codecov bot commented Jul 11, 2019 •
edited
Loading

Codecov Report

Merging #653 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
- Coverage     100%   99.92%   -0.08%     
==========================================
  Files          15       15              
  Lines        2720     2725       +5     
  Branches      534      536       +2     
==========================================
+ Hits         2720     2723       +3     
- Misses          0        2       +2

@samuelcolvin
Copy link
Member
samuelcolvin commented Jul 11, 2019

Please create an issue to discuss this first. Such big changes can't just be implemented directly as a PR.

@abdusco
Copy link
Contributor Author
abdusco commented Jul 11, 2019

Ah sorry, good idea :)

@abdusco abdusco mentioned this pull request Jul 11, 2019
Closed
tiangolo
tiangolo reviewed Jul 13, 2019
View reviewed changes
Copy link
Contributor
@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

I like this! 🎉

I would suggest adding a section in the docs explaining how to use it and adding a small caveat about @dmontagu 's comment: #654 (comment)

@abdusco
Copy link
Contributor Author
abdusco commented Jul 19, 2019

@tiangolo Updated docs with a warning

dmontagu
dmontagu reviewed Jul 19, 2019
View reviewed changes
dmontagu
dmontagu reviewed Jul 19, 2019
View reviewed changes
abdusco
abdusco commented Jul 19, 2019
View reviewed changes
abdusco
abdusco commented Jul 19, 2019
View reviewed changes
dmontagu
dmontagu reviewed Jul 19, 2019
View reviewed changes
dmontagu
dmontagu reviewed Jul 19, 2019
View reviewed changes
@samuelcolvin
Copy link
Member
samuelcolvin commented Jul 20, 2019

Away from my computer, so ignore this if it's covered in the code.

If pytest is giving you trouble, add a simple python test like the mypy external tests and add it to make.

samuelcolvin
samuelcolvin reviewed Jul 23, 2019
View reviewed changes
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.

LGTM, but please add a section to HISTORY.rst and also add a raw python test called perhaps tests/assert_test.py that tests an assert statement passing and failing and adding it to the make tests recipe.

@samuelcolvin
Copy link
Member
samuelcolvin commented Aug 5, 2019

any chance we could get this fixed? I would like to get everything currently pending on the next release so I can work exclusively on v1.

@samuelcolvin samuelcolvin added this to the Version 1 milestone Aug 6, 2019
@samuelcolvin
Copy link
Member
samuelcolvin commented Aug 6, 2019

This is missed release v0.32, let's now include it in v1. That wasn't my original plan, but probably makes sense since it's quite a big conceptual change (even if it's backwards compatible).

@samuelcolvin
Copy link
Member
samuelcolvin commented Aug 6, 2019

please can you rebase and move the history change to changes/ as per #719.

@dmontagu
Copy link
Contributor
dmontagu commented Aug 11, 2019

I think I've addressed the outstanding issues in https://github.com/dmontagu/pydantic/tree/assert_statement_support

I opened a PR against @abdusco 's branch (though it involved a rebase against master which I guess might cause issues). @samuelcolvin let me know if you want me to just make a separate PR.

@samuelcolvin
Copy link
Member
samuelcolvin commented Aug 11, 2019

@dmontagu as a collaborator, you can probably push directly to @abdusco's branch.

Can you try that, if you can't, I'll do it.

dmontagu
dmontagu reviewed Aug 11, 2019
View reviewed changes
tests/try_assert.py Outdated
fail_test(test_name, ['ValidationError was not raised'])


def fail_test(test_name: str, description: List[str]):
Copy link
Contributor
@dmontagu dmontagu Aug 11, 2019

Choose a reason for hiding this comment

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

I added a little bit of pretty error reporting to make it easier for contributors to realize what is going wrong if this test is failing. @samuelcolvin Let me know if you take any issue with the style here.

dmontagu
dmontagu reviewed Aug 11, 2019
View reviewed changes
dmontagu
dmontagu reviewed Aug 11, 2019
View reviewed changes
samuelcolvin
samuelcolvin reviewed Aug 12, 2019
View reviewed changes
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 LGTM.

@dmontagu
Copy link
Contributor
dmontagu commented Aug 14, 2019

Just realized I never pushed the fixes to the abdusco remote 🤦‍♂️. Sorry for the delay.

@samuelcolvin samuelcolvin merged commit f41d5dc into pydantic:master Aug 15, 2019
alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
@samuelcolvin
specific TZ offset constraints (pydantic#653)
3d3cb7f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

@dmontagu dmontagu dmontagu left review comments

+1 more reviewer

@tiangolo tiangolo tiangolo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Version 1

Development

Successfully merging this pull request may close these issues.

4 participants

@abdusco @samuelcolvin @dmontagu @tiangolo
0