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

Add instance-level validators #372

Closed
wants to merge 2 commits into from
Closed

Add instance-level validators #372

wants to merge 2 commits into from

Conversation

hynek
Copy link
Member

@hynek hynek commented Apr 22, 2018

This is a prerequisite for #301

I have not update examples because this belongs into #370 and I will update the respective docs once this is merged.

@hynek hynek added this to the 18.1 milestone Apr 22, 2018
@codecov
Copy link

codecov bot commented Apr 22, 2018

Codecov Report

Merging #372 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #372   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         830    834    +4     
  Branches      174    175    +1     
=====================================
+ Hits          830    834    +4
Impacted Files Coverage Δ
src/attr/_make.py 100% <100%> (ø) ⬆️

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 57817b2...a5c05df. Read the comment docs.

@Tinche
Copy link
Member

Tinche commented Apr 22, 2018

First thought:

why aren't we supporting this:

@attr.s
class C:
    a = attr.ib()
    b = attr.ib()

    @attr.validator
    def _validate(self):
        if self.a > self.b:
            raise ValueError()

Then I went and read the linked issue.

I think runtime type validation is a cool feature that we should definitely support. However the interface of the proposed solution is ugly to me. If you are an advanced attrs user, and you want to extend attrs, you don't actually need this functionality - wrap attr.s or place a class decorator underneath that would wrap an existing attrs_post_init or add a new one if none found, and do your logic in there.

@hynek
Copy link
Member Author

hynek commented Apr 22, 2018

Hm that seems a bit complicated to me. The idea is that you can write a reusable validator that looks at __annotations__ and uses one of the many available type checkers. That sounds like something that should be possible without wrapping @attr.s?

@Tinche
Copy link
Member

Tinche commented Apr 22, 2018

First of all, I'm not going to stop you merging this in, just thinking out loud. :)

We already have a mechanism for executing code after init is done, it's the post_init.

What if you want an additional instance validator after or before typechecking?

Also note:

@attr.s(validator=typecheck)
class C:
    a: int = attr.ib()

In this scenario, typecheck will have to be a generic function, right? What I mean by this is, all instances will share a single function, and this function will probably call attr.fields and iterate over them. If I was doing this, I would want to eval a specialized function to avoid the overhead. And in that case, you need to either wrap attr.s or do your own class decorator. Since I appreciate code aethethics, I would do (hypothetically):

@typechecked.attr.s
class C:
    a: int = attr.ib()

@hynek
Copy link
Member Author

hynek commented Apr 22, 2018

Well, thinking is what I’m (not – but I totally would) paying you for, so please keep doing that. ;)

I guess it’s true that it might get too slow to do it dynamically. 🤔

Maybe you should hopp over to #301 and make some suggestions (after reviewing my PRs natch), I’m gonna close this for now.

@hynek hynek closed this Apr 22, 2018
@hynek hynek deleted the instance-validator branch April 22, 2018 12:08
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.

2 participants