Conversation
sv3ndk
left a comment
sv3ndk
left a comment
There was a problem hiding this comment.
Thanks for the refactoring :)
A few comments, especially about mutating variables.
trumania/core/relationship.py
Outdated
|
|
||
|
|
||
| class Relations(object): | ||
| class OutgoingRelationships(object): |
There was a problem hiding this comment.
I like the addition of Outgoing, that indeed clarifies the direction.
However I would not use the term relationship in this class since that term has a specific meaning in the context of Trumania => I think that would cause a confusion between Relationship and OutgoingRelationship (maybe suggesting the later is a specific case of the former)
=> how about OutgoingRelations ?
| return {key: merged_value(key) for key in keys} | ||
| if dict1 == dict2: | ||
| for k, v in dict1.items(): | ||
| dict1[k] = value_merge_func(v, v) |
There was a problem hiding this comment.
Why do we need to call value_merge_func() ? why not just returning dict1 ? I just did a quick check, python's == seems to honor __eq__() on the dictionnary values => if == returns true, that means those dicts are equal and I think do not need to be merged
There was a problem hiding this comment.
If the merge function is for example add 1, you still need to apply the merge function.
| for key_to_merge in keys_to_merge: | ||
| old_value1 = dict1.pop(key_to_merge) | ||
| old_value2 = dict2.pop(key_to_merge) | ||
|
|
There was a problem hiding this comment.
Unless really proven to be necessary for optimisations reasons, I would discourage mutating dict1 and dict2 since this is a parameter we receive as argument.
This is because, from the point of view of the caller, the statement a = merge_2_dicts(b, c) is not expected to update b nor c, otherwise the method becomes hard to reason about and hard to use in more complex compositions.
I think that thanks to the order you put in return {**dict1, **dict2, **values_merged} (nice one, I didn't know that syntax :) ), the pop is actually not necessary since the priority is given to the values on the right => if we rephrase that for-loop as follows:
values_merged = {k: value_merge_func(dict1[k], dict2[k] for k in key_to_merge}
We obtain the equivalent logic.
What do you think ?
trumania/core/util_functions.py
Outdated
|
|
||
| keys_to_merge = dict1_set.intersection(dict2_set) | ||
|
|
||
| if len(keys_to_merge) == 0 and value_merge_func is None: |
There was a problem hiding this comment.
I think this should be if len(keys_to_merge) != 0
2 participants
No description provided.