martinvonz added inline comments.
INLINE COMMENTS
> context.py:701-705
> +class abstractfilectx(object):
> + def data(self):
> + raise NotImplementedError
> +
> +class basefilectx(abstractfilectx):
abstractfilectx doesn't seem referenced anywhere else, so just revert this
part? Maybe you meant to make arbitraryfilectx extend it? That would make
sense. If we do, I feel like we should implement the smart cmp() in this
abstract base class to avoid the ugly asymmetry in the implementation otherwise
(a.cmp(b) might be faster or slower than b.cmp(a) and one might perhaps even
crash?).
Perhaps we should even make it a top-level function so it will be easier to
override by extensions that want to add their own subclasses? Something like:
def filectxcmp(fctx1, fctx2):
...
class abstractfilectx(object):
def cmp(self, otherfctx): # don't override in subclasses, wrap
filefctxcmp() instead
return filectxcmp(fctx1, fctx2)
Maybe there's precedence for this kind of thing elsewhere in Mercurial? Surely
at least elsewhere in Python.
Or maybe I'm just overthinking this and we're pretty sure all call sites will
pass it as arbitraryfilectx.cmp(filectx) and not the other way around, so it
won't be a problem in practice. I'm not even sure I got that right (and I don't
know where overlayfilectx fits in), which seems like a sign that it's best to
have a single cmp() method.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D674
To: phillco, #hg-reviewers
Cc: sid0, martinvonz, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
