[MERGE] readdir, take 2.

Martin Pool mbp at canonical.com
Tue Sep 2 06:06:34 BST 2008

On Tue, Sep 2, 2008 at 2:44 PM, Robert Collins
<robertc at robertcollins.net> wrote:
>> I really think you should define this to actually return the list in
>> order.  Then you can test that it actually does so, and the
>> sorting/stripping won't be pushed into every place that uses it.  Other
>> code that wants to avoid the overhead of sorting will need more pyrex,
>> but can also avoid the wrapping/unwrapping, which may(?) be comparably
>> expensive.
> Well, that pushes the sort down into the pyrex; os.listdir() is also
> unsorted on output. This is also a an internal function. It seems to be
> tying things too closely together to do this, and neither John nor
> Andrew commented on this [perhaps they missed it, or perhaps they feel
> its ok].

I'm coming to this from Andrew's comment that there is a funny smell
in the test, that indicates the api is not right.

If someone else put up code where the main purpose of the change was
not actually tested I think you would reject it, and not be convinced
by someone else having passed it.

>> I'm not sure what the other pyrex tests do, but this definitely seems
>> like a case where we should be indicating that some tests were not run.
> Why? if the extensions aren't built, test stipple, or a failure, won't
> get them built. PQM should probably require that they built successfully
> up front in the make file, not down at this level.

One of the ideas of the test-not-run framework, which I believe you
wanted, was that it would be obvious if things are not being tested.
Showing test stipple may remind people to do whatever is necessary to
get them built.  I believe at the moment the makefile will still pass
even if the extensions were not built.

At any rate I'm reluctant to add now tests that can silently not run
because if we do want to tighten it up later it may be hard to find
them.  But if it's not obvious how to do it the other way nevermind
for now.

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

More information about the bazaar mailing list