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

Convert assigned mapping objects to Config #6

Closed
vduseev opened this issue Oct 12, 2020 · 1 comment
Closed

Convert assigned mapping objects to Config #6

vduseev opened this issue Oct 12, 2020 · 1 comment
Assignees
Labels
bug Something isn't working scope/docs Documentation
Projects
Milestone

Comments

@vduseev
Copy link
Owner

vduseev commented Oct 12, 2020

When assigning values to existing or new keys, do we miss converting them to Config object? We don't miss it when we use merge method.

self.my.custom.key = { "a": { "b": 3 } }

The dictionary should be converted to Config object upon assignment.

@vduseev vduseev added bug Something isn't working scope/docs Documentation labels Oct 12, 2020
@vduseev vduseev added this to To do in ilexconf Oct 13, 2020
@vduseev
Copy link
Owner Author

vduseev commented Oct 19, 2020

Note and some thoughts for myself/maintainers in the bright future ...

So, it turns out, defaultdict acts in a very bizarre way when we try to intervene with the __setitem__ and __setattr__ behavior.

First try in implementation of this ticket

Here is how the code looked when defaultdict was the base class of Config:

    def __getitem__(self, item):
        if isinstance(item, str) and "." in item:
            key, subkey = item.split(".", maxsplit=1)
            return dict.__getitem__(self, key).__getitem__(subkey)
        else:
            return dict.__getitem__(self, item)

    def __getattr__(self, attr):
        return dict.__getitem__(self, attr)

    def __setitem__(self, item, value):
        value = self._parse(value)  # newly added to implement this ticket
        if isinstance(item, str) and "." in item:
            key, subkey = item.split(".", maxsplit=1)
            dict.__getitem__(self, key).__setitem__(subkey, value)
        else:
            dict.__setitem__(self, item, value)

    def __setattr__(self, attr, value):
        value = self._parse(value)  # newly added to implement this ticket
        dict.__setitem__(self, attr, value)

I've added the value = self._parse(value) line to both __setitem__ and __getitem__ methods and started getting very weird results in this test:

cfg = Config()
cfg["f2"]["g3"]["h3"] = 123
assert cfg["f2.g3.h3"] == 123

Debugging

Without newly added line it all works as it did before this ticket. With the value parsing it stops working. I tried to debug this for a few hours with no luck. Here is what happens during assignment of 123:

  1. 'f2' is requested from cfg, but it does not exist, so lambda is invoked within defaultdict that returns an empty Config{}.
  2. Value of cfg is updated, so that now it equals Config{'f2': Config{}}
  3. 'g3' is requested from empty config that was returned. Same thing happens, new Config{} is generated and returned.
  4. Value of cfg has to be updated again at this point to be equal to Config{'f2': Config{'g3': Config{}}} but it DOES NOT 🤷‍♂️
  5. Curiously, this step is working correctly when we do not temper with the value assignment using _parse and cfg gets correctly updated.
  6. Now, obviously, we can't continue anything because cfg just equals Config{'f2': Config{}} for the rest of the test.

Hypothesis

At this point I started to suspect that there is something about the empty generated Config{} in value argument that gets passed into the __setitem__ method. What if defaultdict's factory generates empty Config{} object in such a way, that CPython knows, that cfg is this new object's parent?

Now, what if, CPython uses this reference inside this new object generated with factory to later properly upsert it into the parent cfg?

Obviously, when I temper with the value argument in __setitem__ method using _parse I naturally get rid of the "old" value and instead generate the "new" value variable with no reference to the parent and etc. This way defaultdict fails to properly upsert new value.

I tried to look into defaultdict implementation in CPython to understand its behavior but that's beyond my capabilities 😅

What if, instead, we base the Config class on just a dict and implement the defaultdict functionality ourselves. In that way we'll be able to precisely control what happens when non existing key is requested from Config.

And, surprise surprise, it works 🎉

Updated code

Here is how it looks after. Notice the _dd_getitem method that replaces the dict.__getitem__(self, key) and implements defaultdict's functionality.

    def __getitem__(self, item):
        if isinstance(item, str) and "." in item:
            key, subkey = item.split(".", maxsplit=1)
            return self._dd_getitem(key).__getitem__(subkey)
        else:
            return self._dd_getitem(item)

    def __getattr__(self, attr):
        return self._dd_getitem(attr)

    def __setitem__(self, item, value):
        value = self._parse(value)
        if isinstance(item, str) and "." in item:
            key, subkey = item.split(".", maxsplit=1)
            self._dd_getitem(key).__setitem__(subkey, value)
        else:
            dict.__setitem__(self, item, value)

    def __setattr__(self, attr, value):
        value = self._parse(value)
        dict.__setitem__(self, attr, value)

    def _dd_getitem(self, item):
        if item not in self:
            dict.__setitem__(self, item, Config())
        return dict.__getitem__(self, item)

ilexconf automation moved this from To do to Done Oct 19, 2020
@vduseev vduseev added this to the Version 1.0 milestone Nov 13, 2020
@vduseev vduseev self-assigned this Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working scope/docs Documentation
Projects
Development

No branches or pull requests

1 participant