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

Revert recursive evolve #806

Merged
merged 5 commits into from
May 6, 2021
Merged

Revert recursive evolve #806

merged 5 commits into from
May 6, 2021

Conversation

hynek
Copy link
Member

@hynek hynek commented May 6, 2021

Seems like the current implementation breaks a non-trivial amount of use cases.

So this reverts it and adds a regression test – lmk if I got it right (also the description of the test).

I welcome suggestions how to do it properly in separate PRs.

@progval @staaam @sscherfke

Fixes #804

@progval
Copy link

progval commented May 6, 2021

LGTM, thanks

You might also want to add a regression test where obj2b is a dict; to make sure one can still replace an attr class with a dict. But it seems like a pretty niche use-case, so it might not be worth it.

@hynek
Copy link
Member Author

hynek commented May 6, 2021

Can you quickly provide one please?

@progval
Copy link

progval commented May 6, 2021

import attr

@attr.s
class Cls1:
    param1 = attr.ib()


@attr.s
class Cls2:
    param2 = attr.ib()


obj2a = Cls2(param2="a")
obj2b = {"foo": 42, "param2": 42}

obj1a = Cls1(param1=obj2a)
obj1b = attr.evolve(obj1a, param1=obj2b)

print(obj1b)

@hynek
Copy link
Member Author

hynek commented May 6, 2021

Thanks!

@hynek
Copy link
Member Author

hynek commented May 6, 2021

(If someone finds the time to contribute a copy-pasted attr.evolve_recursive() with appropriate tests/docs until tomorrow we could push a release that doesn't take anything away. 😇 Unfortunately, I'm busy for the rest of the day.)

@hynek hynek merged commit f10d050 into main May 6, 2021
@hynek hynek deleted the revert-759 branch May 6, 2021 13:26
@sscherfke
Copy link
Contributor

I’m afraid I’m too busy with home schooling and work, too. :-/

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.

attr.evolve no longer supports setting an attr object as attribute
3 participants