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

Support assert statements inside validators #653

Merged
merged 13 commits into from Aug 15, 2019
Merged

Support assert statements inside validators #653

merged 13 commits into from Aug 15, 2019

Conversation

abdusco
Copy link
Contributor

@abdusco abdusco commented Jul 11, 2019

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 codecov bot commented Jul 11, 2019

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
Owner

@samuelcolvin 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 abdusco commented Jul 11, 2019

Ah sorry, good idea :)

Copy link
Sponsor Collaborator

@tiangolo tiangolo left a comment

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 abdusco commented Jul 19, 2019

@tiangolo Updated docs with a warning

tests/test_validators.py Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
pydantic/error_wrappers.py Outdated Show resolved Hide resolved
pydantic/error_wrappers.py Outdated Show resolved Hide resolved
tests/test_validators.py Outdated Show resolved Hide resolved
pydantic/error_wrappers.py Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Owner

@samuelcolvin 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.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

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
Owner

@samuelcolvin 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
Owner

@samuelcolvin 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
Owner

@samuelcolvin samuelcolvin commented Aug 6, 2019

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

@dmontagu
Copy link
Collaborator

@dmontagu 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
Owner

@samuelcolvin 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.

fail_test(test_name, ['ValidationError was not raised'])


def fail_test(test_name: str, description: List[str]):
Copy link
Collaborator

@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.

tests/try_assert.py Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Owner

@samuelcolvin samuelcolvin left a comment

otherwise LGTM.

Makefile Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
tests/try_assert.py Outdated Show resolved Hide resolved
tests/try_assert.py Show resolved Hide resolved
@dmontagu
Copy link
Collaborator

@dmontagu 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 samuelcolvin:master Aug 15, 2019
10 checks passed
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.

None yet

4 participants