[BUGFIX] Fix Context initialization with another Context object

[dannymeijer]

Description

The init method of the Context class incorrectly updated the kwargs making it return None. This means that calls to Context containing another Context object, would previously fail.

Related Issue

Closes #159

How Has This Been Tested?

Several unit tests were added to cover the intended behavior - this was previously not covered

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[nogitting]

Warning

The change itself is not wrong but the Context class needs to be reviewed (and related tests as well).

[dannymeijer]

[!WARNING]

The change itself is not wrong but the Context class needs to be reviewed (and related tests as well).

Can you elaborate on what you mean? Not sure I follow

[nogitting]

Can you elaborate on what you mean? Not sure I follow

For example, do you expect the following
assert {"a": 1, "b": 2, "c": 3} == Context(Context({"a": 1, "b": 2}), Context({"c": 3}))
to be True?

[nogitting]

And here is some Context magic: make to_dict a one-liner such that it simply return self.__dict__, nothing else, and see what happens.

[dannymeijer]

Can you elaborate on what you mean? Not sure I follow

For example, do you expect the following assert {"a": 1, "b": 2, "c": 3} == Context(Context({"a": 1, "b": 2}), Context({"c": 3})) to be True?

Yes, this is intended. Idea is that Context is additive in nature. Meaning, if you initiate a Context from other existing contexts, that they get 'soft merged' - more proper merge behavior is expected to be controlled through the merge method though.

[dannymeijer]

And here is some Context magic: make to_dict a one-liner such that it simply return self.__dict__, nothing else, and see what happens.

This won't work. The idea is that you want any nested instance of Context to also be made into dicts - hence the code as it sits. Feel free to experiment though, if you wish :)

[nogitting]

Yes, this is intended. Idea is that Context is additive in nature

I am not talking about the additive aspect. Do you expect assert Context({"a": 1}) == {"a": 1} to be True (i.e. context-dict equality)?

This won't work. The idea is that you want any nested instance of Context to also be made into dicts - hence the code as it sits. Feel free to experiment though, if you wish :)

Did you run the tests with that one-liner? Is "This won't work" an assumption or a confirmation?

[mikita-sakalouski]

LGTM

[dannymeijer]

Yes, this is intended. Idea is that Context is additive in nature

I am not talking about the additive aspect. Do you expect assert Context({"a": 1}) == {"a": 1} to be True (i.e. context-dict equality)?

Yup - this works. Context is a subclass of Mapping (from collections.abc import Mapping) - Mapping behaves like a dictionary and hence reports like a dict in an direct equals compare

This won't work. The idea is that you want any nested instance of Context to also be made into dicts - hence the code as it sits. Feel free to experiment though, if you wish :)

Did you run the tests with that one-liner? Is "This won't work" an assumption or a confirmation?

Yes. example:

nested_context = Context(a=Context(b=Context(c=42)))
# {'a': {'b': {'c': 42}}}

for key, value in nested_context.__dict__.items():
    print(f"{key = }")
    print(f"{value = }")
    print(f"{type(value) = }")
# key = 'a'
# value = {'b': {'c': 42}}
# type(value) = <class 'koheesio.context.Context'>

for key, value in nested_context.to_dict().items():
    print(f"{key = }")
    print(f"{value = }")
    print(f"{type(value) = }")
# key = 'a'
# value = {'b': {'c': 42}}
# type(value) = <class 'dict'>

[nogitting]

Well, I see it is already merged, but I'll say this anyway:

def test_to_dict(context, expected):  # <= this is not a reliable test
    # Call the method to be tested
    result = context.to_dict()
    # Check that the result is a dictionary with the correct values.  # <= this is not a reliable check
    assert isinstance(result, dict)  # because result can be a dict with contexts inside
    assert result == expected  # <= this evaluates to True
    assert context == result == expected  # <= and so does this (because of your own explanation regarding eq)

And if you modify to_dict like this:

def to_dict(self) -> Dict[str, Any]:
    return self.__dict__

Not only this one but all your context-related tests will still succeed.

Let that sink in.