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
Deep merge ( aka jQuery.extend() ) #25
Comments
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. |
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! |
I see your point. I'll keep this issue open, and get back to it when I've thought about it for a while 👍 |
Perfect, I'll watch this so I can be notified when it's done :) thanks! |
NP, thanks for contributing @julianxhokaxhiu! |
Suggestion: Consider using using the plus operator ('+') for merging addict with a dictionary / with other addicts. but you can implement "+", in a similar way:
In that case, you could write: Which makes perfect sense (to me, and to some others). |
Wow, amazing suggestion! +1 |
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}} |
Would you replace or reject the value of Do you append the string, or replace it? Does I think |
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 |
OOPS, i didn't think of merging different data-types :( I just tested your (sabhiram's) code in jQuery's Extend (with "true" as first argument). singletons were repalced with the newer dictionary, as in update(). But it's not that obvious, isn't it? |
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. |
+1 to extend |
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! |
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 |
Hey @the4tress, we have not yet added it to addict. But we just got a new 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! |
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 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 |
So, at first glance, I must say that it looks like a very thorough job. Well done, @the4tress 👍 |
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! |
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 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. |
@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 |
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. |
Yesterday I was coding some stuff in Python and I was searching for something that is doing a deep merge of two native dictionaries, and I found this https://www.xormedia.com/recursively-merge-dictionaries-in-python/
Is working awesomely and is doing exactly what jQuery.extend() is doing also. Are you going to merge this awesome stuff in your library as well? The only that I missed while I was reading the README was exactly this.
I'm pretty much sure that this will make it more and more awesome :) Thanks in advice!
The text was updated successfully, but these errors were encountered: