| Submitter | Siddharth Agarwal |
|---|---|
| Date | April 10, 2013, 6:21 p.m. |
| Message ID | <645bee3e5241dd659dae.1365618118@sid0x220> |
| Download | mbox | patch |
| Permalink | /patch/1269/ |
| State | Superseded, archived |
| Headers | show |
Comments
On Wed, Apr 10, 2013 at 11:21 AM, Siddharth Agarwal <sid0@fb.com> wrote: > dicthelpers.diff: compare against default for missing values > > This is not only a bit faster, but also aligns with caller's expectations > better since we can legitimately have manifestdict's _flags set to '' > instead > of unset. > This is all well and good, but it's now looking like a special-purpose function that does exactly what manifestmerge needs, rather than a function that belongs in a notionally general purpose module?
On 04/10/2013 11:29 AM, Bryan O'Sullivan wrote: > > On Wed, Apr 10, 2013 at 11:21 AM, Siddharth Agarwal <sid0@fb.com > <mailto:sid0@fb.com>> wrote: > > dicthelpers.diff: compare against default for missing values > > This is not only a bit faster, but also aligns with caller's > expectations > better since we can legitimately have manifestdict's _flags set to > '' instead > of unset. > > > This is all well and good, but it's now looking like a special-purpose > function that does exactly what manifestmerge needs, rather than a > function that belongs in a notionally general purpose module? I think that even in the general case this behaviour is better than the old one. It prevents returning values like ('', ''), which callers wouldn't be able to distinguish from ('', not present) or (not present, '') anyway.
On Wed, Apr 10, 2013 at 11:36 AM, Siddharth Agarwal <sid0@fb.com> wrote: > I think that even in the general case this behaviour is better than the > old one. It prevents returning values like ('', ''), which callers wouldn't > be able to distinguish from ('', not present) or (not present, '') anyway. > > Hmm, okay.
Patch
diff -r 99a410e82b35 -r 645bee3e5241 mercurial/dicthelpers.py --- a/mercurial/dicthelpers.py Sat Apr 06 19:50:03 2013 -0700 +++ b/mercurial/dicthelpers.py Wed Apr 10 10:49:20 2013 -0700 @@ -11,23 +11,23 @@ def diff(d1, d2, default=None): This includes keys that are present in one dict but not the other, and keys whose values are different. The return value is a dict with values being pairs of values from d1 and d2 respectively, and missing values - represented as default.''' + treated as default, so if a value is missing from one dict and the same as + default in the other, it will not be returned.''' res = {} if d1 is d2: # same dict, so diff is empty return res for k1, v1 in d1.iteritems(): - if k1 in d2: - v2 = d2[k1] - if v1 != v2: - res[k1] = (v1, v2) - else: - res[k1] = (v1, default) + v2 = d2.get(k1, default) + if v1 != v2: + res[k1] = (v1, v2) for k2 in d2: if k2 not in d1: - res[k2] = (default, d2[k2]) + v2 = d2[k2] + if v2 != default: + res[k2] = (default, v2) return res diff -r 99a410e82b35 -r 645bee3e5241 tests/test-dicthelpers.py --- a/tests/test-dicthelpers.py Sat Apr 06 19:50:03 2013 -0700 +++ b/tests/test-dicthelpers.py Wed Apr 10 10:49:20 2013 -0700 @@ -49,5 +49,11 @@ class testdicthelpers(unittest.TestCase) 'c': ('baz', 456), 'd': (456, 'quux')}) + # check that we compare against default + self.assertEqual(diff(d1, d2, 'baz'), {'a': ('foo', 'foo2'), + 'd': ('baz', 'quux')}) + self.assertEqual(diff(d1, d2, 'quux'), {'a': ('foo', 'foo2'), + 'c': ('baz', 'quux')}) + if __name__ == '__main__': silenttestrunner.main(__name__)