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

Unstructuring regression for optional generic types #465

Closed
rymndhng opened this issue Nov 29, 2023 · 4 comments · Fixed by #466
Closed

Unstructuring regression for optional generic types #465

rymndhng opened this issue Nov 29, 2023 · 4 comments · Fixed by #466
Labels
regression This is an inintended change for the worse.
Milestone

Comments

@rymndhng
Copy link

  • cattrs version: 23.2.0
  • Python version: 3.10.10
  • Operating System: osx

Description

I upgraded cattr from 22.2.0 -> 23.2.0 and I noticed a difference in behaviour for unstructuring optional generic types.

I've attached a minimally reproducing script that shows the differences in output between the two versions

What I Did

See this code:

import attrs
import cattrs

from typing import Generic, Optional, TypeVar

T = TypeVar("T")

@attrs.define
class A:
    foo: str

@attrs.define
class B(Generic[T]):
    a: T

@attrs.define
class BOptional(Generic[T]):
    a: Optional[T]

@attrs.define
class BOptionalPipe(Generic[T]):
    a: T | None

a = A("bar")

b = B(a)
b_optional = BOptional(a)
b_pipe = BOptionalPipe(a)


print(cattrs.unstructure(b))
print(cattrs.unstructure(b_optional))
print(cattrs.unstructure(b_pipe))

On cattrs 22.2.0, this prints:

{'a': {'foo': 'bar'}}
{'a': {'foo': 'bar'}}
{'a': {'foo': 'bar'}}

On cattrs 23.2.0, this prints:

{'a': {'foo': 'bar'}}
{'a': A(foo='bar')}
{'a': A(foo='bar')}

I noticed that dispatching on a generic type parameter returns the identity function, whereas previously, this would dispatch to the value type. It seems like there's a missing hook for handling typevars with optional.

cattrs.register_unstructure_hook_func(
    typing_inspect.is_typevar, cattrs.unstructure
)
@Tinche Tinche added the regression This is an inintended change for the worse. label Nov 29, 2023
@Tinche
Copy link
Member

Tinche commented Nov 29, 2023

Hm, confirming. Let me see if I can fix this for 23.2.3.

@Tinche Tinche linked a pull request Nov 30, 2023 that will close this issue
@Tinche
Copy link
Member

Tinche commented Nov 30, 2023

Fixed and tests added. I'll release 23.2.3 later today most likely.

@rymndhng
Copy link
Author

rymndhng commented Dec 1, 2023

thanks @Tinche !

@Tinche Tinche added this to the 23.2 milestone Dec 22, 2023
@Tinche
Copy link
Member

Tinche commented Dec 22, 2023

This was fixed, closing now

@Tinche Tinche closed this as completed Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression This is an inintended change for the worse.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants