Deep merge ( aka jQuery.extend() ) about addict

[mewwts]

That sure is interesting functionality, but I'm not sure it belongs on the Dict class. If it's to be included I think I'd prefer something like:

>>> from addict import Dict, extend
>>> a = Dict({...})
>>> b = Dict({...})
>>> c = extend(a, b)

where extend works on all dicts, and resides outside the Dict class.
However, perhaps there are more fitting Python modules to include this in? If not, then we perhaps should include it here 👍

[julianxhokaxhiu]

Honestly I've searched all around the web for something like that, and I only found that snippet. Rather than including 10 modules just to get one thing from one, and one from the other, I would liketo have a full-blown library that just does all the stuff :)

I would love also something like:

>>> from adddict import Dict
>>> a = Dict({...})
>>> b = Dict({...})
>>> c = a.extend(b, ...)

Cleaner and also easier to understand :) Just my two cents, but thanks for getting this in consideration!

[mewwts]

I see your point. I'll keep this issue open, and get back to it when I've thought about it for a while 👍

[julianxhokaxhiu]

Perfect, I'll watch this so I can be notified when it's done :) thanks!

[mewwts]

NP, thanks for contributing @julianxhokaxhiu!

[shula]

Suggestion:

Consider using using the plus operator ('+') for merging addict with a dictionary / with other addicts.
'+' is not implemented for standard dictionaries:
{}+{}
(TypeError: unsupported operand type(s) for +: 'dict' and 'dict')

but you can implement "+", in a similar way:

class Dict:
    ...
    def __add__(self, d):
        self.update(extend(self, d))    # totally NOT a working example

In that case, you could write:
new_addict = addict_obj + normal_dict

Which makes perfect sense (to me, and to some others).

[julianxhokaxhiu]

Wow, amazing suggestion! +1

[sabhiram]

What would be the expected behavior for this test case?

    prop = Dict({'a': 1, 'b': 'string', 'c': [1,2,3], 'd': {'a': 1}})
    result = prop + {'a': 2, 'c': [1], 'b': 'another string', 'd': {'b': 2}}

[sabhiram]

Would you replace or reject the value of a from the rhs?

Do you append the string, or replace it?

Does c get replaced or extended? A string is just an array of chars :)

I think d is the most straightforward with d == {'a': 1, 'b': 2}

[sabhiram]

Here is a hacked up version:

    @classmethod
    def _merge(self, lhs, rhs):
        for key, value in lhs.iteritems():
            if not key in rhs:
                rhs[key] = value
            elif isinstance(value, dict):
                self._merge(value, rhs[key])
        return rhs


    def __add__(self, rhs):
        return self._merge(self.to_dict(), rhs)

And a test:

def test_plus_operator_with_dict(self):
        prop = Dict({'a': 1, 'b': 'string', 'c': [1,2,3], 'd': {'a': 1}})
        result = prop + {'a': 2, 'c': [1], 'b': 'another string', 'd': {'b': 2}}
        self.assertDictEqual(result, {'a': 2, 'b': 'another string', 'c': [1,2,3,1], 'd': {'a': 1, 'b': 2}})

Let me know how you guys want to go forward, I can clean up and send a PR

[shula]

OOPS, i didn't think of merging different data-types :(
it turns merging into a very UNDEFINED method, that can be implemented in different ways, each with its own reasons.

I just tested your (sabhiram's) code in jQuery's Extend (with "true" as first argument).
it MERGED the list items; i.e. [1,2,3] + [1] ==> [1,2,3] and also [1] + [1,2,3] ==> [1,2,3] .
You suggested to EXTEND the lists, i.e. [1]+[1,2,3] ==> [1,2,3,1](although i don't see where it's done in your code snippet).

singletons were repalced with the newer dictionary, as in update().
i'd expect sets to be merged, too.

But it's not that obvious, isn't it?

[sabhiram]

Yea we could end up special casing all those types I suppose, but it would stop being pythonic I feel. I suppose we could allow some flexibility in what is merged and what is replaced, but again adds complexity in the caller.

[debuggerpk]

+1 to extend

[mewwts]

Hey guys,

Great discussion going on here. As I've previously stated, I'm not entirely certain where I stand on this. To me, the standard dict update is quite clear, whereas I feel that the the extend is a bit hard to grasp, see @sabhiram's and @shula's comments. With that said, I guess the extend could be a nice complementary function to the update, if the functionality and use case is clearly documented. I welcome pull requests and further discussion around the subject.

Again, thanks for contributing!

[the4tress]

I stumbled across this thread (and addict) while trying to find a solution to my own version of $.extend().

I see that you added it to addict, but what was the outcome of merging two different types? For my version I just overwrite the first type with the second. So if new.a = ['a','list'] and newer.a = 'a string' then newest = extend(new, newer), newest.a = 'a string'.

[mewwts]

Hey @the4tress, we have not yet added it to addict. But we just got a new update function from #44, and I think addict is ready for this as well. I encourage everyone (@julianxhokaxhiu, @shula and @sabhiram, @the4tress +++) to work on it if they have the time. I will probably have a look myself.

I am not sure how to answer this question as it is not yet implemented, so we could def. need help in planning the method and it's output. Thanks!

[the4tress]

I forked and added a slightly modified version of my extend(). I'm not sure how to update the object though. Right now I'm using a = a.extend() when I should just use a.extend().

My version also has some list options. There are several examples in the way-too-long block comment.

Let me know what you think. This is my first time contributing to somebody else's project so go easy on me. I'll clean up and PEP8 the code later.

Here is my version: https://github.com/the4tress/addict/blob/master/addict/addict.py

[mewwts]

So, at first glance, I must say that it looks like a very thorough job. Well done, @the4tress 👍
I'd like to take some time to go through the code, and in the meantime encourage you to write some test cases for this. As I take a look, I'd also like to engage @burk, @sabhiram, @shula and @julianxhokaxhiu get their opinions on this implementation.

[julianxhokaxhiu]

Sorry @mewwts this period I'm really having no time to spend on this, but I'll look at this asap. But it's a good thing that now we have something concrete to analyze :)

What I can say for now is that my expectations are that is working like jQuery extend is working. But if it has plus features like a smarter way to merge, than it's more than welcome.

I'll give you a more dev friendly feedback asap :) Thanks in advice for everything guys!

[the4tress]

Sorry, between a new job, school and twin two-year-olds I can't commit any time to this for a few weeks. I know this probably needs a lot of work and maybe has some features you don't really need/want, but I was just hoping to get a sample out there for you guys to improve on.

I take my CCNA Sec in a couple weeks. After that I should have more time to write tests and clean up the code.

What am I doing wrong where I can't use a.extend()?

Also, I just realized my examples in the comment block don't really match up. I was testing as I was writing at the same time but it should give an idea how its supposed to work.

[mewwts]

@the4tress @julianxhokaxhiu no worries, I'll try to squeeze in a couple of hours on this in the coming week. The only issue is that I'm not the one requesting this feature so the usecases aren't completely clear to me. I'm more used to using assign in Javascript.

[the4tress]

Isn't assign part of ES6? I haven't played with it much, but I don't think it does a deep merge. I could be wrong.