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

Covert to defaultdict instead of plain dict #519

Closed
4l1fe opened this issue Mar 9, 2024 · 4 comments · May be fixed by #588
Closed

Covert to defaultdict instead of plain dict #519

4l1fe opened this issue Mar 9, 2024 · 4 comments · May be fixed by #588
Milestone

Comments

@4l1fe
Copy link

4l1fe commented Mar 9, 2024

  • cattrs version: 23.2.3
  • Python version: 3.11.2
  • Operating System: Ubuntu 22.04

Description

I expect the method SelectorConfig.load() to convert the toml text to the defaultdict type in SelectorConfig.properties. Instead, it is converted as dict

How to configure structurizing to make it recognizing the exact type which is DefaultDict[str, LineStringProperties] ?

What I Did

config_converter = TomlkitConverter()

@attrs.define
class LineStringProperties:
    pinned: bool = False
    comment: str = ''
    theme_group: str = LIGHT_THEME


@attrs.define
class SelectorConfig:
    properties: DefaultDict[str, LineStringProperties] = attrs.field(factory=lambda: defaultdict(LineStringProperties))

   @staticmethod
    def load(config_path: Path) -> 'SelectorConfig':
        if not config_path.exists():
            return SelectorConfig()
        
        text = config_path.read_text()
        config = config_converter.loads(text, SelectorConfig)
        return config
@Tinche
Copy link
Member

Tinche commented Mar 10, 2024

Hi,

that's an interesting question. I think nobody's asked for it over the years (or I've forgotten about it); we should probably support it since it's a standard library thing.

First you need a predicate function to see if a type is a defaultdict. Note that on 3.9+ (you're on 3.11) you don't need to use typing.DefaultDict, you can just use collections.defaultdict in type hints.

Something like this:

from collections import defaultdict
from typing import DefaultDict, get_origin

def is_defaultdict(type: Any) -> bool:
    """Is this type a defaultdict?"""
    return is_subclass(get_origin(type), (defaultdict, DefaultDict))

Now you need a structure hook factory. We can wrap an existing hook factory, cattrs.gen.make_mapping_structure_fn. You'll need the c Converter to be in scope.

from functools import partial
from typing import get_args

from cattrs.gen import make_mapping_structure_fn
from cattrs.dispatch import StructureHook

def structure_defaultdict_factory(type: type[defaultdict]) -> StructureHook:
    value_type = get_args(type)[1]
    return make_mapping_structure_fn(type, c, partial(defaultdict, value_type))

And then you tie to all together:

c = TomlkitConverter()
c.register_structure_hook_factory(is_defaultdict, structure_defaultdict_factory)

Now you can structure defaultdicts. Since defaultdicts take an arbitrary lambda when created it won't work for all defaultdicts, but should for most.

I'll look into adding this to the next version.

@Tinche Tinche added this to the 24.1 milestone Mar 10, 2024
@4l1fe
Copy link
Author

4l1fe commented Mar 10, 2024

Thank you

Hi,

that's an interesting question. I think nobody's asked for it over the years (or I've forgotten about it); we should probably support it since it's a standard library thing.

First you need a predicate function to see if a type is a defaultdict. Note that on 3.9+ (you're on 3.11) you don't need to use typing.DefaultDict, you can just use collections.defaultdict in type hints.

Hey, thx, i didn't know it.

The problem

Something like this:

from collections import defaultdict
from typing import DefaultDict, get_origin

def is_defaultdict(type: Any) -> bool:
    """Is this type a defaultdict?"""
    return is_subclass(get_origin(type), (defaultdict, DefaultDict))

Now you need a structure hook factory. We can wrap an existing hook factory, cattrs.gen.make_mapping_structure_fn. You'll need the c Converter to be in scope.

from functools import partial
from typing import get_args

from cattrs.gen import make_mapping_structure_fn
from cattrs.dispatch import StructureHook

def structure_defaultdict_factory(type: type[defaultdict]) -> StructureHook:
    value_type = get_args(type)[1]
    return make_mapping_structure_fn(type, c, partial(defaultdict, value_type))

And then you tie to all together:

c = TomlkitConverter()
c.register_structure_hook_factory(is_defaultdict, structure_defaultdict_factory)

Now you can structure defaultdicts. Since defaultdicts take an arbitrary lambda when created it won't work for all defaultdicts, but should for most.

In short, it works, thx for the working solution, but here what i'm thinking about it:

  • it is very dynamic solution that covers all the defaultdict cases recursively yet is very implicit

  • to bring the implemention into code i have to import many additional dependecies

  • many of the dependencies take time to comprehend their responsibility

  • after that, the factory magic is not intuitive and takes even more time to comprehend 🙂

  • the question i ask myself is how the attrs field's attributes are reused? Are they ignored or partially applicaple or whatever?

  • i ended up with an explicit solution without the dynamic factory, but it doesnt work if i write type hintting. It means that i have to drop cattrs or rely on the dynamic factory ONLY removing the simple, explicit code. Here is code that doesn't work

    def to_defaultdict(default_factory, data):
        obj = defaultdict(default_factory)
        
        for name, properties in data.items():
            obj[name] = default_factory(**properties)
        
        return obj
    
    
    @attrs.define
    class LineStringProperties:
        pinned: bool = False
        comment: str = ''
        theme_group: str = LIGHT_THEME
        
        
    @attrs.define
    class SelectorConfig:
        properties: defaultdict[str, LineStringProperties] = attrs.field(converter=partial(to_defaultdict, LineStringProperties),
                                                                         default=to_defaultdict(LineStringProperties, {}))
    
        @staticmethod
        def load(config_path: Path) -> 'SelectorConfig':
            if not config_path.exists():
                return toml_converter.structure({}, SelectorConfig)
                
            text = config_path.read_text()
            config = toml_converter.loads(text, SelectorConfig)
            
            return config

    The idea here is simply to add an attrs converter.

    The toml sample for better understanding:

    [properties]
    [properties."alabaster_dark.toml"]
    pinned = true
    comment = ""
    theme_group = "light"
    
    [properties."alabaster.toml"]
    pinned = true
    comment = ""
    theme_group = "light"

    it's getting broken on creating obj[name] = default_factory(**properties) inside def to_defaultdict(default_factory, data) raising the error while calling config = toml_converter.loads(text, SelectorConfig):

        |     obj[name] = default_factory(**properties)
      |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      | TypeError: __main__.LineStringProperties() argument after ** must be a mapping, not LineStringProperties
      +------------------------------------
    

    Imcompatibility is having type hinting and an attrs converter at the same time while structuring with toml_converter.structure().

The quesiton

I'll look into adding this to the next version.

You mean the defaultdict case will be covered without the hook factory magic?

@4l1fe 4l1fe closed this as completed Mar 15, 2024
@Tinche
Copy link
Member

Tinche commented Mar 15, 2024

You mean the defaultdict case will be covered without the hook factory magic?

Yeah, simple cases we can handle automatically. I'm working on a PR already.

I can look at your other comments later, really busy with work ATM.

@4l1fe
Copy link
Author

4l1fe commented Mar 15, 2024

You mean the defaultdict case will be covered without the hook factory magic?

Yeah, simple cases we can handle automatically. I'm working on a PR already.

I can look at your other comments later, really busy with work ATM.

No worries

@Tinche Tinche linked a pull request Oct 7, 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