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

A dictionary gets structured into a list without errors/warnings #504

Open
fjarri opened this issue Feb 12, 2024 · 2 comments · May be fixed by #540
Open

A dictionary gets structured into a list without errors/warnings #504

fjarri opened this issue Feb 12, 2024 · 2 comments · May be fixed by #540
Milestone

Comments

@fjarri
Copy link

fjarri commented Feb 12, 2024

  • cattrs version: 23.2.0
  • Python version: 3.10
  • Operating System: macos 13.5.1

Description

>>> cattrs.structure({"a": 1, "b": 2}, List)
['a', 'b']

I would expect it to complain that the dictionary it's trying to structure is not a list. While I understand where this logic may be coming from (a dictionary is an Iterable, so it's fine to convert it to a list), it is losing information this way, which doesn't seem like a good choice for the default behavior. Also the docs don't list Iterable as something that gets structured to a list, and dictionaries are not Sequence.

Is it intentional, and if yes, is there a way to make it more strict, in a way that's composable (that is, so I don't have to define a custom structuring function for list of every type I have)?

@Tinche
Copy link
Member

Tinche commented Feb 13, 2024

Howdy!

You're right, the structure hook for lists (and sequences, mutablesequences, homogenous tuples) is very simple; you can see it here. In the case of no detailed validation, it boils down to a single list comprehension. This is for a comple of reasons:

  • performance
  • if we were to check, it's a little unclear what to check for. A certain JSON library might be guaranteed to return only lists, but we support a wide variety of formats and libraries

Also the docs don't list Iterable as something that gets structured to a list

We don't actually check if the input type is an iterable. We just iterate over it. But I get your point.

is there a way to make it more strict

This is a very good question. In my mind the core job of cattrs is for this to be easy to do.

Here's how you would do this today (let's say you decide to perform an additional validation step with an isinstance against a Sequence):

from collections.abc import Sequence

from cattrs import Converter
from cattrs._compat import is_sequence

c = Converter()


def seq_hook(val, type):
    if not isinstance(val, Sequence):
        raise ValueError(f"Not a {type}")
    return c._structure_list(val, type)


c.register_structure_hook_func(is_sequence, seq_hook)

c.structure({}, list[int])

This will be composable: it will affect all lists. That said it kinda sucks.

  • The predicate in an internal module (cattrs._compat) for historical reasons. It'd be better if it was in a public place.
  • The structure hook is, again, a private method. It's pretty old and hasn't gotten modernized since it was "good enough". It'd be better if it was a hook factory and documented.
  • There's no documentation for it. It'd be better if there was a note about this in the docs.

These are all pretty easy fixes and will make the library materially better, so I'm going to assign this issue to the next release to ensure these get done.

@Tinche Tinche added this to the 24.1 milestone Feb 13, 2024
@fjarri
Copy link
Author

fjarri commented Feb 13, 2024

Thank you for a detailed response! It may also be worth noting in the docs that all Iterables and not just Sequences are structured into lists by default.

@Tinche Tinche linked a pull request May 26, 2024 that will close this issue
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 a pull request may close this issue.

2 participants