Add decorator option for converters

[altendky]

Issue #240

Draft for:

• 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.
• Updated documentation for changed code.
  • New functions/classes have to be added to docs/api.rst by hand.
  • Changes to the signature of @attr.s() have to be added by hand too.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in our __init__.py file.
• Documentation in .rst files is written using semantic newlines.
• 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
• Test against the new setattr feature
• Consider eliminating the Converter class
• 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 ebroecker/canmatrix@a889352#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]

040f2b9 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))

[hynek]

Thank you Kyle for staying at the ball, I can only guess how this must be annoying at this point.

Since I’m busy rn I’d like to ask the other fans of this issue to have looks at it.

Please do not expand the scope anymore though, the puck stops here and we have to find an end.

[sscherfke]

LGTM. However, since this is my first review for an attrs PR, someone else should take a look at it, too. :)

[altendky]

@hynek, annoyed? No... :] But I do have to get my head back into attrs a bit. I worked a bit on the setattr stuff last night. There are a couple more points I've added to the 'Draft for:' list in the OP. Not scope creep, I don't think, just dealing with changes in attrs since this was originally written and the consistency with validators.

And I just realized I hadn't set this back to draft... Getting that now.

[sscherfke]

And I just realized I hadn't set this back to draft... Getting that now.

Why? Is something still missing or are you just waiting for a reply from @hynek ?

[altendky]

@sscherfke, I try to keep a todo list in the first post of my PRs. As I identify things to do I add them, then check them off when done. I just took a few days to work on getting a new pyqt5-tools out this morning. Assuming it doesn' t need immediate fixup, maybe I'll get back to this. If you have any suggestions about the open issues in my 'Draft for:' list, I'm happy for some help. :]

[hynek]

Taking off the milestone to take off the pressure. Take all the time you need, thank you staying with us!

[alonme]

Hey @altendky @hynek - what is status for this PR?
Any help needed?

[SubeCraft]

Any merge update ?

[altendky]

Anyone is welcome to branch off my work and try to get it fully ready for merge. I don't know what more has come up in the last few years, but I did leave a list of needed work, or at least things to consider, before I thought it would be ready.

Sadly, I don't make it out to community development much these days so I don't expect to be working on this. Nor do I get to work with attrs, which also is sad. :[

[hynek]

Anyone is welcome to branch off my work and try to get it fully ready for merge. I don't know what more has come up in the last few years, but I did leave a list of needed work, or at least things to consider, before I thought it would be ready.

Yeah I'm keeping it open, because I've started a few times to implement the three-argument converters myself and using is for inspiration, but if you care about performance, it's a bit trickier than it seems on the first sight and something always came up. :|

Sadly, I don't make it out to community development much these days so I don't expect to be working on this. Nor do I get to work with attrs, which also is sad. :[

💔 – thank you for all your contributions!

[altendky]

At least I'm disappearing on PR #404...