[MERGE] inode sorted stats in status

Andrew Bennetts andrew at canonical.com
Tue Sep 2 03:32:28 BST 2008


bb:tweak

I don't share John's concerns about merging this even though it's a small
improvement.  There isn't really that much code here, even though there is a
Pyrex module, so I'm happy for this to be merged as a modest incremental
improvement.

As far as the code itself, aside from John's remarks, I only have a couple of
remarks to make.

Robert Collins wrote:
[...]
> === added file 'bzrlib/_readdir_pyx.pyx'
[...]
> +            if not (name[0] == dot and (
> +                (name[1] == 0) or 
> +                (name[1] == dot and name [2] == 0))

There's a whitespace nit in “name [2]”.

> === modified file 'bzrlib/tests/test_osutils.py'
[...]

> +def load_tests(standard_tests, module, loader):
> +    """Parameterize readdir tests."""
> +    to_adapt, result = split_suite_by_re(standard_tests, "readdir")
> +    adapter = TestScenarioApplier()
> +    from bzrlib import _readdir_py
> +    adapter.scenarios = [('python', {'read_dir':_readdir_py.read_dir})]

PEP 8 implies that colons should have whitespace after them (and most Python
code I've seen does so).

[...]
> +    def test_readdir(self):
> +        tree = [
> +            '.bzr/',
> +            '0file',
> +            '1dir/',
> +            '1dir/0file',
> +            '1dir/1dir/',
> +            '2file'
> +            ]
> +        self.build_tree(tree)
> +        expected_names = ['.bzr', '0file', '1dir', '2file']
> +        # read_dir either returns None, or a value
> +        read_result = self.read_dir('.')
> +        if read_result[0][0] is None:
> +            # No innate sort:
> +            expected_keys = [None, None, None, None]
> +            expected = zip(expected_keys, expected_names)
> +            expected.sort()
> +            self.assertEqual(expected, sorted(read_result))
> +        else:
> +            # Check that the names we need are present in the second
> +            # column
> +            names = sorted([result[1] for result in read_result])
> +            self.assertEqual(expected_names, names)
> +        

If statements in test methods are a bad smell.

Why not just:

        ...
        read_result = self.read_dir('.')
        # Check that the names we need are present in the second
        # column
        names = sorted([result[1] for result in read_result])
        self.assertEqual(expected_names, names)

If you separately want to assert that the _py version always fills in None as
the first value, then that strikes me as something that belongs in a separate
test (a test specifically for that implementation, rather than all read_dir
implementations).

-Andrew.




More information about the bazaar mailing list