[PATCH 03 of 15 v2] dirstate: create class for status lists
Martin von Zweigbergk
martinvonz at gmail.com
Fri Oct 10 17:21:57 UTC 2014
On Mon, Oct 6, 2014 at 11:20 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 10/05/2014 08:08 AM, Martin von Zweigbergk wrote:
>>
>> @@ -26,6 +26,49 @@
>> def join(self, obj, fname):
>> return obj._join(fname)
>> +class status(tuple):
>> + '''Named tuple with a list of files per status.
>> + '''
>
>
> Hmm. There are several things I would like to point out ... but they seem to
> all be inherited from the Python namedtuple. It might make sense to stay as
> similar as possible but slightly different.
Several things you would like to point out in the docstring? I know
the docstring is very short, but I also don't know what else to put
there. I considered moving the descriptions for the different statuses
from the dirstate.status() method. I might do that in v3.
> Why not use a generic named tuple implementation ... especially on Python
> 2.6+ where it is built-in? There might be good reasons, but they should
> documented.
We have to support Python 2.4-2.5 too, right? Would I have to check
the version (or existence of named_tuple)? Or back-port it? It just
sounds like extra code for little benefit to me, so please explain to
a poor Python newbie.
> I guess one reason is that the standard implementation creates the class as
> source code and execs it. -1 for elegance, +1 for getting the work done.
>
> Another reason is that this implementation defaults to having unspecified
> parameters as empty lists ... but is that really relevant / necessary?
>
>> +
>> + __slots__ = ()
>> +
>> + def __new__(cls, modified=[], added=[], removed=[], deleted=[],
>> unknown=[],
>> + ignored=[], clean=[]):
>
>
> http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments
> - this is the only significant issue I see with this series.
Wow, that's fun. Will use None as default instead.
>> + return tuple.__new__(cls, (modified, added, removed, deleted,
>> unknown,
>> + ignored, clean))
>> +
>> + @property
>> + def modified(self):
>> + return self[0]
>
>
> I think I would prefer to store the members by name (in slots?) and avoid
> doing the array index lookup. That would make it more like an object that
> behaved like a tuple than the opposite.
>
> But these status objects should however never be created in tight loops
> anyway, so these (and other) optimizations might be premature.
That's what I think, as I said in reply to Yuya's email. Since I heard
no complaints, I'll leave it as is until we've seen that it does
matter.
>> + def __repr__(self, *args, **kwargs):
>> + return (('status(modified=%r, added=%r, removed=%r, deleted=%r, '
>> +
>> + 'unknown=%r, ignored=%r, clean=%r)') % self)
>
>
> repr usually looks like '<status yada yada>' - I would expect this to
> follow the same pattern. But ok, this is how Python does it ...
Will change.
More information about the Mercurial-devel
mailing list