Add decorator option for converters

[altendky]

trafficstars

Issue #240

Draft for:

• [x] Added tests for changed code.
• [ ] New features have been added to our Hypothesis testing strategy.
• [ ] Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
  • [ ] ...and used in the stub test file tests/typing_example.py.
• [x] Updated documentation for changed code.
  • [x] New functions/classes have to be added to docs/api.rst by hand.
  • [x] Changes to the signature of @attr.s() have to be added by hand too.
  • [x] Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in our __init__.py file.
• [x] Documentation in .rst files is written using semantic newlines.
• [x] Changes (and possible deprecations) have news fragments in changelog.d.
• [ ] Maybe update the mypy plugin
  • https://github.com/python/mypy/blob/13ae58ffe8bedb7da9f4c657297f0d61e681d671/mypy/plugins/attrs.py
  • tests/typing_example.py:126: error: Unsupported converter, only named functions and types are currently supported
• [x] Test against the new setattr feature
• [x] ~~Consider eliminating the Converter class~~
• [x] Test the passed Attribute
• [ ] Look over again for any more doc changes
• [ ] Resolve the circular import due to attr.setters.converter() needing attr._make.Converter (but attr._make imports attr.setters so...)

[hynek]

I see it’s WIP, just a bunch of random notes to save you some time:

• are you sure we need takes_self in converters? I don’t think so, we don’t have anything similar for the converter option either and you hard-code it too. The diff gets a lot simpler if you “just” use the decorator to set the option and be done with it.
• test docstrings
• newsfragment :)

[altendky]

That's why I put it up here sooner than later, thanks.

I made Converter to be like Factory is for default=. Though the calling side didn't end up so similar I don't think, I need to review that to see about removing the Converter.__call__ part that Factory doesn't have. I'll see how I can simplify that as I learn Factory's implementation better but it is not presently clear to me how the diff would be much different without Converter.takes_self. It seems like just assuming that to be true would only alleviate half of a boolean check/assignment.

converter_self = (
    isinstance(a.converter, Converter) and a.converter.takes_self
)

would roughly become:

converter_self = isinstance(a.converter, Converter)

[altendky]

@hynek, I figured it would make sense to clarify the logic before writing up docs etc. I didn't quite understand how your suggestion would make the diff much simpler. Do you have time to offer a bit more explanation?

I looked at making it so that internally converter is always a Converter but that seems questionable because attr.fields(MyClass).my_attribute.converter would not provide what the developer expects when have attr.ib(converter=my_function). With this change, TestConverter.test_deprecated_convert failed for this reason. Otherwise, this did make for a significant simplification of the diff. I wouldn't expect we would want to hold up progress much based on a deprecated parameter name, but I do expect that aside from that we want to get the same object out of attr.fields(MyClass).my_attribute.converter that we put into converter=.

Then again, perhaps I have just misrepresented my needs. I want both the decorator syntax for converter and also the takes_self aspect similar to default=Factory(). I see how the former does not imply the latter and I'm sorry if that caused confusion. The issue that brought me to this topic this week is https://github.com/ebroecker/canmatrix/commit/a8893523319c4ede32e59ee19bf8420095945e41#r29555978.

[altendky]

I'm not sure if storing a Converter() regardless in names_for_globals is a tidy way to increase consistency in this code while retaining the outward appearance of whatever the caller passed to convert=... or if it's really ugly.

[hynek]

So um I’m confused by your use case, it looks like you really just want a default? What do you need self in the converter for? What I meant is that you should “just” set the converter attribute of CountingAttr and be done with it, which would allow you to touch a lot less code.

So could you elaborate why you think you need self in a converter? Because semantically, it doesn’t make any sense, but I guess I’m missing something?

P.S. The rst in the news fragment is broken. :)

[altendky]

One attribute is float_factory which defaults to float but may also be decimal.Decimal or such. Whatever is passed in for the offset attribute should be converted using the float_factory attribute. Does that explain the desire for self in the converter? It seems possibly a little dirty, but I haven't decided it's bad yet.

As to just making every _CountingAttr just have a Converter? It seems it would be nice if an attribute defined as class C: a = attr.ib(converter=f) would return f from attr.fields(C).a.converter. Hopefully I'm remembering my functions etc correctly at the moment.

[altendky]

https://travis-ci.org/python-attrs/attrs/jobs/401284405#L510

would reformat /home/travis/build/python-attrs/attrs/tests/test_make.py

I don't get this locally and it's not telling me what line etc... but apparently I guessed correctly.

[altendky]

040f2b978147837f8f0f0b0880012da2f43904a6 adds a test for my desired behavior of attr.fields(C).a.converter being the object passed via attr.ib(converter=f). I figure that as long as my code is targeting that, it should have a test. If attrs doesn't care about that functionality then code and tests will both get updated.

[hynek]

• a working attr.fields(C).a.converter is definitely a must
• I can sympathize with needing self for having a factory since I use that pattern in structlog although I’m still not a fan of the complexity it introduces. 🤔 I’d like a weigh in by @Tinche, @wsanchez, and/or @glyph if there’s something missing from the picture.

[glyph]

I honestly don't understand the use-case at all here.

Access to partially-initialized self was a nightmare in the cases where I used it even with validators, for converters this seems worse. Every time I think I want a converter that accesses self, what I actually want is a @classmethod constructor that does the I/O before attempting to construct an instance.

[wsanchez]

Sorry, I thought I'd have this tons-of-free-time summer this year and yet reality disliked my plans.

Anyway, I haven't been following along. Is there an example of that this is meant to achieve? I don't see added docs in the PR or an example in this ticket, so it's not obvious to me what the problem being solved is.

That said, I share the above concerns about anything passing a half-baked self around.

[altendky]

@glyph, the short explanation is that the converter is actually configurable via a 'previous' attribute. The library was hardcoded with float but I wanted to use decimal so I introduced the option to specify the factory to be what the application developer wanted. I would have sworn it was used for other stuff in the class but it appears to just be a converter for a few values. This doesn't seem great, more like we should let the developer apply it themselves or else we just need it elsewhere. :|

I have to admit I was also surprised when I first saw that the default decorator let you have a partially initialized self. I have used that on several occasions IIRC, so I just kind of figured I'd include it here for the use that brought me to work on this as well as consistency with default.

Sorry for not providing docs yet, the reasoning in my head was that I'd write them once we decided what the functionality would be. I see how having them now could help clarify.

[hynek]

I think what Kyle wants it this:

@attr.s
class C:
    type = attr.ib(default=float)
    value = attr.ib(default=0.0). # NB: you can’t even have mandatory attribs with this approach

   @value.converter
    def _(self, val):
        return self.type(val)

And I’m gonna side with @glyph here: passing around half-initialized selfs is something I rather regret so I would strongly prefer to not add more of them. I also feel like converters per se are being overused – at least I noticed it for myself. It really shouldn’t do anything more than int -> float or similar. Once you want logic, you should put that into a @classmethod with a meaningful name.

This is really complicated. 🤔

[glyph]

@altendky So to be as specific as possible, the question is: why don't you do this instead?

@attr.s
class C:
    type = attr.ib()
    value = attr.ib()

    @classmethod
    def new(cls, type, value):
        return cls(type, type(value))

[altendky]

I don't have a problem doing 'something else' like suggested. I implemented the way it is because there was a ticket open for the decorator and because I figured symmetry with the default decorator taking self made sense. Since default taking self is viewed as an error, I understand not repeating it with converter. (I'll have to think about it more before I am sure if I agree it is an error :] )

So, no self... Do we even want a decorator? Aside from my sample use case which is easy enough to find an alternative to (such as in the last comment)

Overall I thought the relationship between an attribute and it's default or converter code seemed nice. Better than a totally arbitrary __attrs_post_init__ or @classmethod but it sounds like I should reconsider that perspective.

[glyph]

I don't have a problem doing 'something else' like suggested. I implemented the way it is because there was a ticket open for the decorator and because I figured symmetry with the default decorator taking self made sense. Since default taking self is viewed as an error, I understand not repeating it with converter. (I'll have to think about it more before I am sure if I agree it is an error :] )

Oh, this is my bad. I should have taken issue with the ticket rather than the PR.

So, no self... Do we even want a decorator? Aside from my sample use case which is easy enough to find an alternative to (such as in the last comment)

Personally I am starting to think that even the decorator for validators was a mistake, one that we might want to avoid repeating. However I think I might still have some of my own uses I should audit first...

Overall I thought the relationship between an attribute and it's default or converter code seemed nice. Better than a totally arbitrary attrs_post_init or @classmethod but it sounds like I should reconsider that perspective.

I think __attrs_post_init__ is worse for cases that compute defaults, because it leaves the object in a partially-initialized state for a while until it gets "fixed up" at the end. However, arbitrary @classmethod I would say is generally better, because it's a function which doesn't even try to create the instance until all the parameters are sorted out.

[hynek]

Yeah so to be clear, we don't blame you for tackling an open issue – we’re still figuring this stuff out in-flight ourselves and I’m very much heartbroken that you probably wasted your time. However it kinda prompted a good discussion and is a good opportunity for some introspection.

I’m gonna share my experience with the related issues over the years:

• converter is almost never what you want. It makes sense for the original intent of ensuring that a number is a float or int or vice versa. I also like to use it to convert strings into URL objects but even that is kinda sketchy. The problem is that it generally becomes less clear how to instantiate the object and has all kind of weirdnesses around itself.
• Passing around half-initialized objects was a bad idea and it already fell on @glyph's foot once, so I don’t want to add any more of that. It sort of makes sense for validators because they tend to be read only and it allows to validate items in relation to each other but I also honestly have to admit that I don't use validators myself a lot. For typing I now have mypy and for everything more complicated it's usually a better idea to take the @classmethod route.
• Obviously a decorator makes no sense if you don't want to pass around self. You can as well write a function; maybe scope it inside the class body.
• __attrs_post_init__ can be quite useful but probably not for what most people think. So far I’ve identified two useful things: 1. initialising things like metrics objects or loggers that live on a class and depend in some way on data on the class: e.g. labels. 2. calling super() if you have to subclass and need to call it. The second use case is really a fringe issue that shouldn't affect to many people but those who need it, really need it. Although it would be nice to know how many people actually use it and how many decided that it’s not worth it in the end.

---

So with a heavy heart, I propose to close this and #240 as won't fix and instead add more documentation to http://www.attrs.org/en/latest/init.html – basically summing up what glyph and I wrote above.

I welcome your comments first though!

[altendky]

My comments about expecting this (concept) to just be accepted weren't meant to express frustration. Rather just that I hadn't put a lot of thought into the concept making sense or not because it seemed to fit into the mold of other pieces etc. :] Closing is fine.

For the discussion though, I still don't see the issue with passing around a partially initialized object to things that are clearly part of initialization. It's just breaking up an __init__ into pieces and just like __init__, order matters. Also like __init__, you get an attribute error if you access another attribute before you define it. Could people get confused about this? Sure. I've gotten confused about much more obvious errors (and maybe this one as well the first time). I guess I see attrs as providing a framework for describing these things (default, converter, etc) rather than totally freeform code like a class method. I am probably also biased because I have built my own things on top of this structure. Serialization, automatic on change signals for PyQt, a PyQt model from attrs classes which uses the converter method for user input conversion. attrs reduces boilerplate... and also offers this framework for easily introspecting attributes to build large frameworks.

As to inheriting? I do that some, I should do it less, but still some. I try not to let Qt's inheritance leak too far but sometimes a bit seems appropriate.

[nyanpasu64]

Why was this rejected?

I load config files from YAML, where user input is generally in the form of str. I would like to use a "converter decorator" to convert Class.field from str to EnumSubclass[field].

But when I pass a Class object to attr.evolve, field is already EnumSubclass, so I must skip EnumSubclass[field] to prevent crashing. (I don't need access to half-initialized self.)

I feel defining an external converter outside the class is ~~unnecessarily bloated...~~ hmm I could just add a EnumSubclass.from_str(Union[str, EnumSubclass]) and use it as a attr.s(converter=).

[hynek]

It wasn't definitely rejected, since it's still open. :) The problem is that it's dawning on me, that converters weren't the best idea in the first place. They are nice if you just need to fix up some simple value, but they get weird really fast, especially in concert with defaults. A classmethod is almost always the better solution.

---

I think – and I'd love input from others on this too – that the better way would be to focus on figuring out #146?

[altendky]

What if attrs created the @classmethod? As another option, I joked recently about attr.with_converter(TheClass)(x, y) as well. Or the opposite such as attr.raw_init(TheClass).

The basic point is that you should be able to create the class without all the conversion and validation, even if you do usually want those activities to occur?

I haven't thought about it much but the hybrid attribute 'handler' seems reasonable. To toss another point into the mix, having the handlers (or converter/validator) applied even after init is sometimes useful. I've got a probably messy thing that creates properties for that.

[glyph]

What if attrs created the @classmethod?

I wouldn't say that this is necessarily a bad idea, but in my mind there are two cases:

1. attrs initializes some attributes directly, and the constructor has no logic beyond that (with the proviso that converters can massage a few things from strings by parsing them or somesuch)
2. there's some complex, interdependent logic for initializing the attributes from some inputs

for the first case, converters are already adequate; for the second, I don't know if there's really much attrs can do to help. When writing __init__s all day long I was constantly thinking "this is tedious, I wish it could be automated!". Now that I write @classmethods on attrs classes all day long, in the simple case, I just… don't write the classmethod at all and don't worry about it.

I'd be interested to hear if anyone else has a specific sense that there's a tedious or automatable portion of writing classmethod constructors.

[glyph]

hmm I could just add a EnumSubclass.from_str(Union[str, EnumSubclass]) and use it as a attr.s(converter=)

We do this in several places in our codebase and it comes out reasonably nicely.

Maybe there's room for a generic if_not_already(Type[O], Callable[[T], O]) -> Callable[[Union[T, input], O] to be somewhere in attrs but I don't think this affects how converters work in any general sense.

[altendky]

@glyph, my point was that presently you can't bypass the converters (correct?). Having attrs provide a way to bypass those steps or putting them in a class method rather than init would make it possible.

[glyph]

my point was that presently you can't bypass the converters (correct?).

As far as I know, yes.

Having attrs provide a way to bypass those steps or putting them in a class method rather than init would make it possible.

Hm. This strikes as squarely within the domain of metaprogramming, ("if you want this, you probably want to call __new__, not __init__"). The given concrete use-case so far (serialization) is definitely just such a thing; you don't want to have to explicitly opt in to having serialization skip initialization steps with optional methods, you want a thing that is going to call attr.fields and have some very defined semantics with respect to what it does with the information it finds there, which either does or doesn't respect the constructor.

[hynek]

my point was that presently you can't bypass the converters (correct?).

As far as I know, yes.

Which is one of the main reason why I think this approach is flawed and shouldn't be pursuit any further.

What if attrs created the @classmethod?

I don't think this is useful TBH. I understand your desire for a raw __init__ (which is what I usually do nowadays) but I don't think code is gonna get any easier if we try to write some meta language to define factory methods. I think that'd just introduce a lot of complexity, make it less readable, and still not fix all use cases.

Sometimes it's OK to write some code. :)

---

The question is whether or how to simplify @glyph's first use case: give some power to __init__ but don't make it too awkward. And I truly believe that #146 is the better way and the solution should put emphasis on code re-use.

I'm very open to suggestions on how to achieve that.

[meereeum]

I would like to echo the request for more fully-featured converters (whether via this or #146) - especially the ability to take self as an arg. (while acknowledging the tradeoff involved in adding another way to interact with a half-baked self..)

Consider this use-case: the initial val is some str. The converter is like a case statement -- has a dictionary of keys (of which val must be one) and values that are functions of previously passed attrs (hence need for self).

I could do this in __attrs_post_init__ but seems clumsy to separate the attr.ib definition from its actual effect.

Not sure if this issue has fallen by the wayside because of lack of contributors or perceived lack of interest, but just wanted to register definite interest !

I have found attrs in general to be very useful and concise, but this would make it feel more natural rather than like I'm sometimes fighting against the varying capabilities of factory vs validator vs converter...

[BrendanSimon]

I would like to echo the request for more fully-featured converters (whether via this or #146) - especially the ability to take self as an arg. (while acknowledging the tradeoff involved in adding another way to interact with a half-baked self..)

Consider this use-case: the initial val is some str. The converter is like a case statement -- has a dictionary of keys (of which val must be one) and values that are functions of previously passed attrs (hence need for self).

I could do this in __attrs_post_init__ but seems clumsy to separate the attr.ib definition from its actual effect.

My use case is to do some processing on the input value. e.g. trim the size of a string to a maximum length. The length is determined by another attribute in the class (self.max_length). I can't do this with a global function as self is not passed to it (with a coverter callable).

Using __attrs_post_init__ can be done but only for instanace initialisation. It wont work if you have a setter for the attribute. e.g. `attr.ib( ..., on_setattr=attr.setters.convert ).

Actually that does work for calling the converter, but I need access to other attributes of the instance.

That's where an @setter.converter would be handy (well as long as the API passes a reference to the instance, like the validator decorator does).

[altendky]

I don't know if it's really better but at this point I pretty much don't use converters or validators (or __attrs_post_init__). If I want to do anything beyond accepting a value and putting it in the attribute I write an @classmethod builder. Or a regular function because I ran into issues with proper type hinting and cls and just trying to allow inheritance without even actually leveraging it.

I kind of feel like there's a way to get a 'just construct the class' thing and a 'validate and convert when constructing' thing built by attrs (I made some kinda crazy examples before concluding I shouldn't). The 'partially constructed self' concerns never completely settled with me. They make sense but at some point there is code that executes in an order and which builds the pieces. Though I guess tying that order to __init__ parameter order which ties it to defaults-first and the order from attrs.to_dict() etc etc might just be too much. Maybe that justifies simply not passing around 'partially constructed self', maybe not.

[altendky]

I suppose I could also say that if interest from those with authority is gotten I could take some time to catch up this PR etc. IIRC, I stopped because it wasn't obvious that it was going somewhere that would be wanted.