-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Generalizing ad-hoc attribute callbacks #146
Comments
Maybe |
My line of thinking was basically that:
Thus, if we can come up with a more sophisticated interface for converters, we could unify converters and validators and get rid of validators. I think the So here is my idea, which may be different than @hynek's in scope. We define a new concept. I'll call it hook, although I'm not really happy with that name, but let's go with it for now. A simple class, to illustrate the concept.
(I think it's telling I had to open attrs docs to look up the exact validator signature to write this.) This results in the following
This can be simplified by having a hook that does both.
So this is basically the same code, except we've eliminated one core concept. I'd say it's a cleaner design. Now, about the differences in the interface. Validators take three arguments: the instance, the attribute, and the value. Converters only take the value. So let's be clever at (I have just realized For backward compatibility, |
So I was thinking (dangerous!). You’re obviously right that the only difference between a validator and convert is the amount of arguments they receive. As for getargspec (and But wouldn’t it make sense to allow a whole pipeline that feeds into each other? e.g. @attr.s
class C:
git_ref = attr.ib(pipe=[str])
@git_ref.pipe
def shorten(self, val):
return val[:6] Kind of like structlog’s processors. I like composable callables. Opinions? @glyph? :D |
This strikes me as a bridge too far :-). A full data-flow abstraction in your attribute definitions? I shudder to think of the confusion. Also, if you really needed this, you could compose it out of simpler pieces. For example: @attr.s
class C:
refify = something_other_than_attrs.pipe()
refify.pipe(str)
@refify.pipe
def shorten(self, val):
return val[:6]
git_ref = attr.ib(convert=refify) However, this seems like a stand-in for something much more complex, which I can't fully understand whether it'd be necessary or not. Consider the much simpler: @attr.s
class C:
git_ref = attr.ib(convert=lambda x: str(x)[:6]) Not only is this considerably shorter, but it has the additional advantage of having a meaningful traceback that points at the offending line if one of its stages were to fail. The |
The conclusion is – of course – an integrated e-mail client! |
I think both of you are right. Having a single (i.e. non-list) “smart” All in all that would just mean that we’d rename convert to pipe, allow the usage as a decorator, and be smart about the arguments of the callables. At that point I don’t think the rename is worth it but usage as a decorator gives nice partity with validator and OTOH we should add the smart arguments feature to validators too to have parity the other way. |
Removing milestone because this is more thinking/bikeshedding than I’m willing to tolerate for 17.1. |
Re: #404 (comment) I like the idea of a general hook from which the user can describe conversion/validation/.... But, what here let's me take a class that I've defined which validates and converts inputs using the proposed approach, and also create an instance of it without those being enforced? Or is the |
OK, I've done more dangerous thinking also in wrt #655 and #709. I think the concept of a pipe is the way to go, but I think we've been thinking from the wrong side. What we/the people want:
Currently we're lacking validators with 1 arg and converters with 3 args and everything is somewhat overlapping. It is a matrix, but it's all about adapting interfaces. So how about this: def ensure_positive_int(value):
if not value >= 1:
raise ValueError("int not positive")
def do_it_all(inst, att, value):
x = int(value)
if not x >= 1:
raise ValueError("int not positive")
return x
@attr.define
class C:
x: int = attr.field(pipe=[attr.Converter(int), attr.Validator(ensure_positive_int)])
# Same:
@attr.define
class C:
x: int = attr.field(pipe=[do_it_all]) The idea here is that:
Obviously, there's also room for us to optimize the One thing I like about this approach is that it makes the order explicit. Currently converters run before validators but that's something you have to know. It might be a bit more verbose than deducing the intent by looking at parameters, but I find it makes everything clearer. OPINIONS? |
I find the intricate mad-genius reasoning here very appealing, but I do feel like I should ask: is If instead, we simply had, say: @attr.define
class C:
x: int = attr.field(
process=lambda inst, att, value: attr.validate(
inst, att, attr.convert(inst, att, value, int), attr.greater_than(0)
)
) at first blush this might seem uglier, but for elaborate pipelines you still get a regular traceback, composition works by functions calling other functions, tracebacks in Sentry and similar tools get all the info they'd usually get, and |
(not to mention that Mypy could be made to work on this much more easily than trying to type-match the adjacent elements in a sequence) |
@wsanchez here's a less deliberately obfuscatory version: def check_x(inst: object, att: Attribute, value: object) -> int:
converted = attr.convert(inst, att, value, int)
is_positive_integer = attr.greater_than(0)
validated = attr.validate(inst, att, value, is_positive_integer)
return validated
@attr.define
class C:
x: int = attr.field(process=check_x) Part of my point here though is exactly this: if you're doing a |
Alternately, method-style: @attr.define
class C:
x: int = attr.field()
@attr.processor(x)
def _process_x(self, att: Attribute, value: object) -> int:
converted = attr.convert(self, att, value, int)
is_positive_integer = attr.greater_than(0)
validated = attr.validate(self, att, value, is_positive_integer)
return validated |
Riffing some more; maybe it would be cleaner to have the things as methods on Attribute: @attr.define
class C:
x: int = attr.field()
@attr.processor(x)
def _process_x(self, att: Attribute, value: object) -> int:
converted = att.convert(self, value, int)
is_positive_integer = attr.greater_than(0)
validated = att.validate(self, value, is_positive_integer)
return validated |
Since we'd have only one type of processors, you'd have to have more than one if you want a stock validator and stock converter. For custom processors it makes no sense of course. @glyph's Obviously we could say |
Random thought before I forget it: since we have |
Re: #655 (comment) Having read this thread I, personally, think that the idea from #146 (comment) is worth considering. It is also similar to what I was thinking of in #655 (comment). The general idea of piping also looks nice and attractive to me, as well as more generalized look on converters and validators. The point I'd want to make about it is that good interface is crucial for it to become widespread. So the solution with BTW, the word "pipe" makes me think of possible syntactic sugar alias with the
Looks a bit like typing exercises, so I'm more in favor of argspec analysis and automatic deduction, if a single fixed function interface doesn't work. Moreover, this will allow to integrate and reuse existing conversion-like functions just as is. |
While 37d38c4 is pretty cool, when working on it it was very obvious to me, that it should be united with converter. After @Tinche pointed it out too, there’s no way back.
IOW, something like this:
Any other ideas?
The downside that it breaks the simplicity of the current implementation and we’d have to treat those callbacks in a special way (instead of just like any other validator).
Thinking of it, it might make sense to make it a different feature altogether (i.e. having a lightweight validator and something more substantial) because requiring to return the new value is a source for errors…in any case it mustn’t be called validator because the semantics would be different.
I’d tend to say that people can just do a
self.x = hex(value)
but that breaks frozen classes and we have that problem already in #120.The text was updated successfully, but these errors were encountered: