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

Allow aggregation of errors from multiple root validators #1571

Closed
beezee opened this issue May 27, 2020 · 4 comments
Closed

Allow aggregation of errors from multiple root validators #1571

beezee opened this issue May 27, 2020 · 4 comments

Comments

@beezee
Copy link
Contributor

beezee commented May 27, 2020

Allow aggregation of errors from multiple root validators

Output of python -c "import pydantic.utils; print(pydantic.utils.version_info())":

pydantic version: 1.5.1
            pydantic compiled: True
                 install path: /usr/local/lib/python3.7/site-packages/pydantic
               python version: 3.7.6 (default, Dec 30 2019, 19:38:28)  [Clang 11.0.0 (clang-1100.0.33.16)]
                     platform: Darwin-18.2.0-x86_64-i386-64bit
     optional deps. installed: ['typing-extensions']

It looks like this line of code is the only thing forcing the first root validator failure on a model to be the only root validator failure to be reported - https://github.com/samuelcolvin/pydantic/blob/master/pydantic/main.py#L918

I know it's subjectively arguable that one root validator is all you need, but a) the fact that in this code we are iterating over a collection of root validators seems to conflict with the idea of limiting to a single root validator and b) this seems like an unnecessary limitation that would only serve to make the library less flexible for users.

Is this a deliberate choice? Are there dire consequences I'm not anticipating that would arise from aggregating all failures for all root validators, same as we do for those of the non-root variety?

I'd be happy to open a PR and update/add any tests/docs whatever other auxiliary changes that would accompany this change in behavior, but I wanted to ask about the motivation and whether such a PR would be welcome before putting in the work.

Thanks.

@samuelcolvin
Copy link
Member

PR, welcome. I guess it was just done for performance but was perhaps a mistake.

This will need to be prominently noted in the docs and changelog.

@beezee
Copy link
Contributor Author

beezee commented May 31, 2020

Thanks @samuelcolvin happy to hear it.

I've opened the PR, would welcome your guidance on where you think this is worth calling out in the docs. My first impression was that this was not really a documented behavior prior, and since the change brings root validators in line with attribute validators, I'm having a hard time coming up with an intuitive way to highlight the new behavior.

@beezee
Copy link
Contributor Author

beezee commented Jun 4, 2020

Linking to the PR #1586

@samuelcolvin let me know if there's anything else I can do to help expedite approval and merge.

@fsramalho
Copy link

I was exploring pydantic and just found that. Thanks @beezee for the PR to fix that 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants