Issue 11709044: Datastructure (usually meta rev) diff function (Closed)
|
Created:
10 years, 10 months ago by ana.balica Modified:
10 years, 9 months ago Reviewers:
thomas.j.waldmann Visibility:
Public. |
DescriptionDatastruct (or meta) diff is a useful function for displaying changes in the UI and in mail notifications.
Consider having a nested datastructure with the following possible data types: dict, list, unicode, int, float, long, NoneType. The end result is represented as a list of tuples, each marking an addition or a deletion in a sorted order.
Patch Set 1 #
Total comments: 18
Patch Set 2 : Split tests and use Undefined class in diff function #
Total comments: 2
Patch Set 3 : Update diff function #
Total comments: 35
Patch Set 4 : Restructured tests #
Total comments: 1
Patch Set 5 : Change tests #
Total comments: 14
Patch Set 6 : #MessagesTotal messages: 13
you didn't run that code, right? :) https://codereview.appspot.com/11709044/diff/1/MoinMoin/constants/keys.py File MoinMoin/constants/keys.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/constants/keys.py#new... MoinMoin/constants/keys.py:122: # diff markers somehow this doesn't fit here. those "keys" here are keys present in metadata (or in the index, which is mostly equivalent). so, move your constants either to your own module (if they are primarily used there) or to another constants/* module if you expect widespread usage of them. https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/_tests/test_diff... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/_tests/test_diff... MoinMoin/util/_tests/test_diff_datastruct.py:11: from MoinMoin.util import diff_datastruct from MoinMoin.util.diff_datastruct import diff https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/_tests/test_diff... MoinMoin/util/_tests/test_diff_datastruct.py:16: def test_diff(self): you can keep this as test_complex_diff, if you like. but I'ld like to additionally see multiple very simple and very short tests that systematically test all code paths / all datatypes / all operations. each test would focus on just one single aspect (and try not to test again stuff that is already tested by other tests). https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/_tests/test_diff... MoinMoin/util/_tests/test_diff_datastruct.py:18: u"key1": u"value1", people who like to type less / have easier typing tend to write dicts like this: d1 = dict( key1=u"value1", key2=u"value2", ) or same in single line for the rather simple tests. you would need one additional test about unicode vs str keys. if you can make sure they behave the same, you won't need to write u"key" everwhere. https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.py File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:8: from MoinMoin.constants.keys import INSERT, DELETE, REPLACE see comment there. i only saw 2 of these there, btw. https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:12: """ Get the diff of 2 datastructures (usually 2 meta revisions) usually 2 revision meta dicts https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:18: that represent pieces of the diff that can be used to format a diff https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:32: if isinstance(d2[key], NoneType): if d2[key] is None: (as I already pointed out, there is only 1 None instance ever, it is a singleton) https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:35: changes.extend(diff(None, d2[key], keys)) also, why are you handling this typecheck (for None) on this level? you could also just always do the recursive diff() call and handle it there. hmm, i see it now: you have None for 2 different semantics: a) you really got None as a value b) you use None as a placeholder for "it didn't exist before / after" I suggest you use an own object of class UNDEFINED to do that cleaner. https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:40: changes.extend(diff(d1[key], None, keys)) see above https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:89: raise TypeError("Unsupported data type {0}".format(type(d))) that code in line 76..89 is somehow duplicate functionality. maybe we could do it much simpler by handling a UNDEFINED -> definedvalue transition just like emptyobjectofsametype -> definedvalue. (and similar for definedvalue -> UNDEFINED using definedvalue -> emptyobjectofsametype) it's not perfectly clean as one can not see the difference between undefined and empty then, but is maybe good enough for our usage. the "emptyobject" could be just t() with t=type(other).
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/1/MoinMoin/constants/keys.py File MoinMoin/constants/keys.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/constants/keys.py#new... MoinMoin/constants/keys.py:122: # diff markers maybe move to misc.py? https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/_tests/test_diff... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/_tests/test_diff... MoinMoin/util/_tests/test_diff_datastruct.py:18: u"key1": u"value1", On 2013/07/24 12:41:09, Thomas.J.Waldmann wrote: > you would need one additional test about unicode vs str keys. if you can make > sure they behave the same, you won't need to write u"key" everwhere. I thought the function should accept only unicode. Otherwise we should check for isinstance(d1, basestring), no? https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.py File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:89: raise TypeError("Unsupported data type {0}".format(type(d))) On 2013/07/24 12:41:09, Thomas.J.Waldmann wrote: > maybe we could do it much simpler by handling a UNDEFINED -> definedvalue > transition just like emptyobjectofsametype -> definedvalue. I don't quite get exactly what you mean... What is emptyobjectofsametype and emptyobjectofsametype -> definedvalue transition? If we are using an undefined object instead of None, then we skip some extra checks, which is good. There is duplication. I tried to first check for a defined value, and then check for Undefined inside, but the duplication persisted already in every defined value if.
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/1/MoinMoin/constants/keys.py File MoinMoin/constants/keys.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/constants/keys.py#new... MoinMoin/constants/keys.py:122: # diff markers On 2013/07/24 15:44:44, ana.balica wrote: > maybe move to misc.py? move it to diff_datastruct module for now. we can still search a nice place in MoinMoin.constants.* IF this is used from many places. https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/_tests/test_diff... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/_tests/test_diff... MoinMoin/util/_tests/test_diff_datastruct.py:18: u"key1": u"value1", The json VALUES are only unicode. Not sure if the json deserializer also uses unicode-only for the KEYS. But even if, supporting / testing str for the KEYS should be no issue and makes the tests simpler. Also consider this: $ d = {'foo': 1) $ type(d.keys()[0]) <type 'str'> $ d[u'foo'] 1 $ d = {u'foo': 2} $ type(d.keys()[0]) <type 'unicode'> $ d['foo'] 2 You see, python does same no matter whether key is str or unicode. $ hash('foo') -4177197833195190597 $ hash(u'foo') -4177197833195190597 Note: i replaced >>> by $ due to codereview not liking >>>. the reason why most stuff in constants.keys is unicode is that whoosh insists on getting unicode (not str). https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.py File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:89: raise TypeError("Unsupported data type {0}".format(type(d))) > What is emptyobjectofsametype and > emptyobjectofsametype -> definedvalue > transition? If you have no key x in d1, but you have key x in d2 and d2[x] is type dict, emptyobjectofsametype would mean dict() (== the empty dict). You would use this emptyobject instead of the value you do not have in d1. > If we are using an undefined object instead of None, > then we skip some extra checks, which is good. Yup. Also we avoid a not-working case of in-band signalling.
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/2/MoinMoin/util/_tests/test_diff... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/2/MoinMoin/util/_tests/test_diff... MoinMoin/util/_tests/test_diff_datastruct.py:22: def test_diff_none(self): The samples with other types like (bool, int, float, long) are sharing the same execution path with None. But if you consider that it's better to test them also, I will write some extra tests. https://codereview.appspot.com/11709044/diff/2/MoinMoin/util/diff_datastruct.py File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/2/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:14: def diff(d1, d2, basekeys=None): The execution path didn't change much, since i was unclear with some suggestions. The use of an Undefined object made the path cleaner though.
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.py File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/1/MoinMoin/util/diff_datastruct.... MoinMoin/util/diff_datastruct.py:89: raise TypeError("Unsupported data type {0}".format(type(d))) On 2013/07/24 17:23:07, Thomas.J.Waldmann wrote: > If you have no key x in d1, but you have key x in d2 and d2[x] is type dict, > emptyobjectofsametype would mean dict() (== the empty dict). Got it. Will try to see if it leads to less duplication.
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:13: DELETE = u"delete" btw, as this is only used for the tuples you build, you could just do: INSERT, DELETE = range(2) ehrm, why is this in the tests module and not in the code module? you can just import it from there, as you do with the diff function. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:22: assert diff_ == [] anything wrong with?: assert diff(d, d) == [] https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:24: def test_diff_none(self): this is not primarily testing None, but rather adding and removing a key from a dict. could be 2 tests, one for add key, one for remove key. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:27: expected_diff = [(DELETE, [u"key0"], None), (INSERT, [u"key2"], None)] is the "u" needed? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:28: diff_ = diff(d1, d2) if you want to go this route, use something like "expected =" and "got =". but maybe you could just directly compare? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:31: def test_diff_unicode(self): this is both testing adding/removing a key and diffing unicode. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:33: d2 = dict(key1=u"same value", key0=u"value", key2=u"new value") more readable: d1 = dict(removed=u"removed value", same=u"same value") d2 = dict(same=u"same value", added=u"added value") https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:34: expected_diff = [(DELETE, ["key0"], u"old "), (INSERT, ["key2"], u"new value")] the first one is a bit hard to interpret maybe we don't want to do that. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:40: d2 = dict(key0=[3, 4]) instead of giving dicts, you could also directly give the lists. you could do that for all supported datatypes, btw. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:45: def test_diff_dict(self): see above. just give a dict, not a dict of dicts. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:60: def test_diff_keys(self): test_str_unicode_keys https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:62: d2 = dict() use same style, {} https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:70: for i, d in enumerate(d1): did you want to use zip()? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:13: Undefined = namedtuple("Undefined", []) unusual idea. the usual way maybe would rather be: class UndefinedType(object): """ ... """ Undefined = UndefinedType() does your way have any advantages over the usual way? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:29: if isinstance(d1, Undefined) and isinstance(d2, (dict, list, unicode, )): what's with NoneType, integer, long, unicode, bool? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:31: elif isinstance(d2, Undefined) and isinstance(d1, (dict, list, unicode, )): same here https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:33: if isinstance(d1, dict): what type can d2 be here? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:45: elif isinstance(d1, (unicode, list, )): what type can d2 be here? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:61: isinstance(d2, (NoneType, bool, int, long, float, )): or? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:67: if d1 != d2: what type can d1 and d2 be here?
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:13: DELETE = u"delete" On 2013/07/24 22:39:57, Thomas.J.Waldmann wrote: > INSERT, DELETE = range(2) why mark INSERT as 0 and DELETE as 1? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:40: d2 = dict(key0=[3, 4]) On 2013/07/24 22:39:57, Thomas.J.Waldmann wrote: > instead of giving dicts, you could also directly give the lists. > > you could do that for all supported datatypes, btw. Testing of each type will also include testing same values, added, removed. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:45: def test_diff_dict(self): On 2013/07/24 22:39:57, Thomas.J.Waldmann wrote: > see above. just give a dict, not a dict of dicts. So we don't want to test for nested dicts? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:13: Undefined = namedtuple("Undefined", []) On 2013/07/24 22:39:57, Thomas.J.Waldmann wrote: > does your way have any advantages over the usual way? No, just one-liner. Should the class have a doc string? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:29: if isinstance(d1, Undefined) and isinstance(d2, (dict, list, unicode, )): On 2013/07/24 22:39:57, Thomas.J.Waldmann wrote: > what's with NoneType, integer, long, unicode, bool? Unicode is present. For integer: say d2 = 3, then d1 = type(d2)() => d1 = 0 That will lead to: (DELETE, [], 0) (INSERT, [], 3) But the truth is, it is just (INSERT, [], 3). OR d2 = 0, d1 = type(d2)() +> d1 = 0 changes = [] In fact it is (INSERT, [], 0) Same situation will happen with NoneType, long and bool. Their default types, are real values. Or have I gone in the wrong direction? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:33: if isinstance(d1, dict): On 2013/07/24 22:39:57, Thomas.J.Waldmann wrote: > what type can d2 be here? Anything but Undefined. I will change it to: if all(isinstance(d, dict) for d in (d1, d2)) https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:61: isinstance(d2, (NoneType, bool, int, long, float, )): On 2013/07/24 22:39:57, Thomas.J.Waldmann wrote: > or? all(isinstance(d, (...) for d in (d1, d2))
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:13: DELETE = u"delete" > why mark INSERT as 0 and DELETE as 1? it's just a specific, differentiable value, no semantics. if you feel better, you can also have DELETE, INSERT = range(2). https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:40: d2 = dict(key0=[3, 4]) On 2013/07/25 07:47:12, ana.balica wrote: > On 2013/07/24 22:39:57, Thomas.J.Waldmann wrote: > > instead of giving dicts, you could also directly give the lists. > > > > you could do that for all supported datatypes, btw. > > Testing of each type will also include testing same values, added, removed. you can only test add/remove if you have a dict and some specific type as value in there. that's already a (mildly) complex text. the easiest test is just giving one of the supported data types, without having it nested as value in a dict. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:45: def test_diff_dict(self): > So we don't want to test for nested dicts? sure you want, but you first want to do simple tests, then more complex ones. you can much more easily identify issues if a simple test fails than trying to debug a failing complex / nested test. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:13: Undefined = namedtuple("Undefined", []) > Should the class have a doc string? There needs to be something in the class. The choice between a docstring nicely documenting it and a useless "pass" should be obvious. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:29: if isinstance(d1, Undefined) and isinstance(d2, (dict, list, unicode, )): > (DELETE, [], 0) > (INSERT, [], 3) Hmm, yeah, it's not that great just pulling that out of the air. It would be only OK if we for sure know that the internal default when not present is really 0. > OR > > d2 = 0, d1 = type(d2)() +> d1 = 0 > changes = [] That would be ok if we knew that the internal default is 0. Then explicitly stating 0 doesn't make a difference. That's a quite fundamental problem and not that much different for different types. For unicode (or list or dict) it is also only legitimate to assume the empty object of that type if we know that is somehow the default used when it is undefined. https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:33: if isinstance(d1, dict): > > what type can d2 be here? > > Anything but Undefined. I will change it to: > if all(isinstance(d, dict) for d in (d1, d2)) that's correct, but for such simple cases I'ld prefer if isinstance(d1, dict) and isinstance(d2, dict):
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/21001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/21001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:25: assert diff(v1, v2) == expected that's correct, but why not just: tests = [(None, None, []), (Undefined, None, [(INSERT, [], None)]), (None, Undefined, [(DELETE, [], None)]), ] for d1, d2, result in tests: assert diff(d1, d2) == result or even: assert diff(None, None) == [] assert diff(Undefined, None) == [(INSERT, [], None)] assert diff(None, Undefined) == [(DELETE, [], None)] you know "K.I.S.S."? :) maybe reconsider the other zip()s also.
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:13: DELETE = u"delete" On 2013/07/25 08:42:05, Thomas.J.Waldmann wrote: > if you feel better, you can also have DELETE, INSERT = range(2). how about just keeping it "insert" and "delete" strings just like SequenceMatcher does? https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/16001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:29: if isinstance(d1, Undefined) and isinstance(d2, (dict, list, unicode, )): On 2013/07/25 08:42:05, Thomas.J.Waldmann wrote: > That's a quite fundamental problem and not that much different for different > types. For unicode (or list or dict) it is also only legitimate to assume the > empty object of that type if we know that is somehow the default used when it is > undefined. I think the problem may be different for dict, list and unicode, because of the way the diff is computed. Consider d1 = dict(), d2 = dict(key=u"value") AND d1 = Undefined, d2 = dict(key=u"value"). The diff for both versions will be (INSERT, ["key"], u"value"), because we don't care for the presence of the an empty dict, list or unicode. We can't ensure the same for other types, besides dict, list and unicode. Values like 0, False, 0L 0.0 are valid values.
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:22: (None, Undefined, [(DELETE, [], None)])] if you have 1 tuple per line, it's much easier to read. https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:41: tests = [(long(1), long(1), []), 1L ? (on could even think about dropping the "long" test (not the long type in the isinstance check in the diff code, though), because int mostly behaves like long (and vice versa) when necessary anyway - and there is no long in py3 any more) https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:59: (u"diff1", u"diff2", [(DELETE, [], u"1"), (INSERT, [], u"2")])] think we should change the behaviour here, just telling delete and u"1" isn't enough information. should rather: a) treat unicode like int... (not like a list) or b) somehow display old/new value and indicate where it changed. https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:66: ([1, 2], [2, 3], [(DELETE, [], [1]), (INSERT, [], [3])])] maybe add a testcase with a different number of list elements (like you test different length unicode above). https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:73: (dict(old=1), Undefined, [(DELETE, ["old"], 1)])] same here with different amount of keys. https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:86: d2 = {} maybe have some same keys in d2, so you see whether it really treats str and unicode keys as the same (you don't really see it with your test), like: d1 = {u"same1": "same2", "same2": "same2", "removed: "removed"} d2 = {"same1": "same1", u"same2": "same2", u"added": "added"} https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:91: tests = [(u"foo", True), ((1, 2, ), (3, 4, )), (dict(key=(1, 2, )), dict())] why shall tests[1] raise? https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/diff_datastr... File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:49: elif all(isinstance(d, (unicode, list, )) for d in (d1, d2)): maybe expand that in same way as 37 and move check for unicode from 49 to 64. https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:64: elif any(isinstance(d, (NoneType, bool, int, long, float, )) for d in (d1, d2)): did you draw a type matrix to check whether your "if/elif" covers all the combinations? https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:70: if type(d1) == type(d2): are the 2 lines above equivalent to saying "elif type(d1) == type(d2): ..." ? https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:78: raise TypeError("Unsupported data type {0}".format(type(d1))) maybe tell both the types of d1 and d2. use exactly same text as in 75?
Sign in to reply to this message.
https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:59: (u"diff1", u"diff2", [(DELETE, [], u"1"), (INSERT, [], u"2")])] On 2013/07/26 11:57:12, Thomas.J.Waldmann wrote: > think we should change the behaviour here, just telling delete and u"1" isn't > enough information. > > should rather: > a) treat unicode like int... (not like a list) > or > b) somehow display old/new value and indicate where it changed. Agree, sequence matcher gives some portions, which are not understandable unless some context is given. Maybe treat as int, since meta isn't supposed to carry much of text information. Moreover meta contains ids (hashes) displayed as unicode. If we use matching, then we can run into having some repeated patterns, which might be confusing for some users. A complete DELETE and INSERT is much more clean in this case. https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_diff_datastruct.py:91: tests = [(u"foo", True), ((1, 2, ), (3, 4, )), (dict(key=(1, 2, )), dict())] On 2013/07/26 11:57:12, Thomas.J.Waldmann wrote: > why shall tests[1] raise? With the current code, it raises "Unsupported data type tuple". With the next patch it will raise "Unsupported diff between tuple and tuple" :) https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/diff_datastr... File MoinMoin/util/diff_datastruct.py (right): https://codereview.appspot.com/11709044/diff/32001/MoinMoin/util/diff_datastr... MoinMoin/util/diff_datastruct.py:64: elif any(isinstance(d, (NoneType, bool, int, long, float, )) for d in (d1, d2)): On 2013/07/26 11:57:12, Thomas.J.Waldmann wrote: > did you draw a type matrix to check whether your "if/elif" covers all the > combinations? did it now. yes, it covers all the combinations for the 9 types (including UndefinedType) used in this method.
Sign in to reply to this message.
|
