[MERGE] inode sorted stats in status

[Martin Pool]

On Tue, Sep 2, 2008 at 12:36 PM, Robert Collins
<robertc at robertcollins.net> wrote:
> On Tue, 2008-09-02 at 12:32 +1000, Andrew Bennetts wrote:
>> If statements in test methods are a bad smell.

I agree with Andrew that they are.  (Which is not to say they should
never be used.)  Specifically as in this case it's a statement that
either of two outcomes is acceptable.

My concerns when I see one would be:

* It may not be clear to a reader or reviewer whether both branches
are active, or when.  It might be that there is no longer any live
code that exercises one of them.  There is no test that the inums are
actually returned.

* It raises a question whether the api should really be defined to
return two different possible results, and whether that will risk bugs
or complexity in non-test callers.

In this case all you are effectively asserting is that if the first
sort key is none then they all are, and it would be easier to say that
directly.  Separately, regardless of sort keys, you want all the right
filenames.

Why not do the sort and strip off the inode numbers within this function?

> Are they? I think its entirely appropriate when an interface can
> optionally do something that the test matches that optional capability.
>
> Trying to avoid that makes the test not fit the code (and vice versa)
> and _that_ is a bad smell.

Also looking at your patch:

+def read_dir(path):
+    """Like os.listdir, this reads a directories contents.
+
+    There is a C module which is recommended which will return
+    a file kind in the second element of the returned tuples.
+
+    :param path: the directory to list.
+    :return: a list of (None, basename) tuples.
+    """
+    return [(None, name) for name in os.listdir(path)]

spelled "directory's"

As far as I can see the C version does not return the file kind, and
anyhow it's not in the second element.

+/*
+ *  Bazaar-NG -- distributed version control
+ *
+ * Copyright (C) 2006 by Canonical Ltd

That was a long time ago ;-)

-- 
Martin <http://launchpad.net/~mbp/>