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

__attrs_init__() #731

Merged
merged 14 commits into from
Jan 23, 2021
Merged

__attrs_init__() #731

merged 14 commits into from
Jan 23, 2021

Conversation

indigoviolet
Copy link
Contributor

@indigoviolet indigoviolet commented Dec 13, 2020

Here's a first pass for #725. @hynek, please take a look to see if you like this approach, and I can then do all the polish work below. Tests were passing at least locally, for py38. Please ignore all the poetry-related noise, I'll get rid of that.

It's a bit ugly for a few reasons:

  1. I wanted to have {args} in both the auto-generated __init__ and __attrs_init__, but didn't want to repeat or call that code twice; and it seems non-trival to factor out.

  2. We have to always generate __attrs_init__ (so that the user-provided __init__ can refer to it), but conditionally generate __init__ as before.

  3. Because of (2), Factory now calls _make_init (even though it has defined __init__), but _make_init refers to Factory, so we need a small hack for this.

I think that __attrs_pre_init__ will be cleaner to implement, and I'm not sure I fully see the advantages of __attrs_init__ over __attrs_pre_init__. Thoughts?

Pull Request Check List

This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing left to do. If your pull request is a documentation fix or a trivial typo, feel free to delete the whole thing.

  • 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.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

.envrc Outdated Show resolved Hide resolved
.python-version Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@indigoviolet

This comment has been minimized.

@indigoviolet indigoviolet force-pushed the vi-attr-init branch 2 times, most recently from 6a68a6f to 9030d1e Compare December 13, 2020 01:43
@hynek
Copy link
Member

hynek commented Dec 13, 2020

First an important lifehack: the solution to your .envrc problem is called .git/info/exclude which is a directory-specific ignore file. I use it everywhere too.


As for the PR: it is quite intrusive, are you sure you wouldn't be fine with __attrs_pre_init__ for now? I think that should be a lot easier to implement and has the mentioned upside of having proper types. I wanna do both but the length of the PoC is somewhat daunting.

@indigoviolet
Copy link
Contributor Author

I'm fine with __attrs_pre_init__ as mentioned in the issue and summary above, I was under the impression that you preferred this approach. I will abandon this and replace with a different PR for that instead.

Note, however, that this PR does maintain the types for __init__. In fact that accounts for much of its intrusiveness, since we need to do the same generation for both __init__
and __attrs_init__.

@hynek
Copy link
Member

hynek commented Dec 13, 2020

I was under the impression that you preferred this approach.

Your impression is 100% on point! It's just that we need both anyways and __attrs_pre_init__ seems like the lower hanging fruit?

Note, however, that this PR does maintain the types for __init__.

Maybe I misunderstand, but the point of __attrs_init__ as original intended is that attrs does not write an __init__ at all and provides the user with with an __attrs_init__ instead.

IOW instead of:

@attr.define
class C:
    x: int

    def __attrs_pre_init__(self):
        do_a()
    def __attrs_post_init__(self):
        do_b()

The user can write:

@attr.define(init=some_magic_value_we_need_to_agree_on)
class C:
    x: int
    def __init__(self, x: int) -> None:
        do_a()
        self.__attrs_init__()
        do_b()

Are we on the same page at all or am I being confusing?

@indigoviolet
Copy link
Contributor Author

indigoviolet commented Dec 14, 2020

Maybe I misunderstand, but the point of __attrs_init__ as original intended is that attrs does not write an __init__ at all and provides the user with with an __attrs_init__ instead.

As I understand it now, your idea is: based on the init flag, we will either write __init__ (as before), or instead name that method (minus the call to __attrs_post_init__) as __attrs_init__. That makes sense to me, and I will update accordingly.

I was doing something slightly different: always providing an __attrs_init__ and a default __init__ implementation that calls __attrs_init__() and __attrs_post_init__(), which the user can override -- thus removing the need for the init flag. But your version makes more sense, and I think it will be simpler, since I'm only renaming that function.

Separately:

  1. why do you think it is useful to have __attrs_pre_init__() + __attrs_post_init__() as well as __attrs_init__()? It seems like these two have overlapping functionality - when do you anticipate someone using __attrs_init__() and their own __init__() instead of the pre/post init hooks? I can think of two situations: (a) they want __init__ to take different arguments than their declared attr.ibs() would generate or (b) they need the arguments to do their pre-init work (and we won't pass arguments to __attrs_pre_init__()).

Conversely, why add __attrs_pre_init__() if we have __attrs_init__() - because it can be simpler and the user doesn't have to think about calling __attrs_init__(), __attrs_post_init__()?

  1. Currently init=True means an __init__ is generated, and init=False means __init__ is not. Can we treat init=False as indicating that they want an __attrs_init__? There is also auto_detect -- so: can we inject __attrs_init__ if we are not injecting __init__?

@indigoviolet
Copy link
Contributor Author

indigoviolet commented Dec 14, 2020

This is now ready for review.

@indigoviolet
Copy link
Contributor Author

@hynek just want to check that this is on your radar. no rush but if you happen to know when you might get to it, i won't bug you until then :)

@hynek
Copy link
Member

hynek commented Dec 20, 2020

Yes it is, sorry. I need to get out a structlog release before I can focus on bigger attrs work.

@indigoviolet
Copy link
Contributor Author

ping?

Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delays and thank you for your patience! Overall this is all high quality, but we need to figure out some edge cases together.

src/attr/_make.py Outdated Show resolved Hide resolved
src/attr/_make.py Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
src/attr/_make.py Outdated Show resolved Hide resolved
src/attr/_make.py Show resolved Hide resolved
@hynek hynek added this to the 21.1.0 milestone Jan 19, 2021
Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool,thanks! This is an exciting feature!

We should add some docs to init.rst but that doesn't need to happen here.

@hynek hynek merged commit 654aa92 into python-attrs:master Jan 23, 2021
@hynek
Copy link
Member

hynek commented Jan 23, 2021

So, since you're knee deep in this code: do you think it would be feasible to also add __attrs_pre_init__?

I still think that's more useful for most people, that just want to call super() and not lose the typing information we deliver. 🤔

@indigoviolet indigoviolet deleted the vi-attr-init branch January 24, 2021 03:15
@hynek hynek mentioned this pull request Jan 26, 2021
hynek added a commit that referenced this pull request Apr 6, 2021
Signed-off-by: Hynek Schlawack <[email protected]>
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.

3 participants