[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