launchpad-dev team mailing list archive

[Bjorn Tillenius <bjorn@xxxxxxxxxxxxx>]

On Wed, Dec 09, 2009 at 09:57:51AM -0500, Aaron Bentley wrote:
> Bjorn Tillenius wrote:
> >> +            for attribute in ['st_uid', 'st_gid', 'st_mtime', 'st_nlink']:
> >> +                if getattr(stat_result, attribute, None) is None:
> >> +                    setattr(stat_result, attribute, 0)
> > 
> > This code is very unobvious to me. Why do you have to replace None with
> > 0?
> 
> I don't.  None is not a valid value for any of the named attributes, so
> it's a useful sentry.  If getattr(stat_result, attribute, None) returns
> None, the attribute did not exist at all.

Well, reading the code, it looks like it can be either None or not
exist. How do I know that it can't be None? It would be much clearer if
you either had a comment explaining it, or did:

    missing = object()
    if if getattr(stat_result, attribute, missing) is missing:


> When the input is a MemoryStat, I have to add these attributes because
> lsLine requires them to be set.
> 
> 
> >> +    def test__format_directory_entries_with_MemoryStat(self):
> >> +        """format_directory_entries works with MemoryStat.
> >> +
> >> +        MemoryStat lacks many fields, but format_directory_entries works
> >> +        around that.
> >> +        """
> >> +        t = MemoryTransport()
> >> +        stat_result = t.stat('.')
> >> +        entries = self.sftp_server._format_directory_entries(
> >> +            [stat_result], ['filename'])
> >> +        self.assertEqual(list(entries), [
> >> +            ('filename', 'drwxr-xr-x    0 0        0               0 '
> >> +             'Jan 01  1970 filename',
> >> +             {'atime': 0,
> >> +              'gid': 0,
> >> +              'mtime': 0,
> >> +              'permissions': 16877,
> >> +              'size': 0,
> >> +              'uid': 0})])
> >> +        self.assertIs(None, getattr(stat_result, 'st_mtime', None))
> > 
> > What's this last assert for? Why don't you care whether stat_result has
> > an st_mtime attribute?
> 
> I do care, very much.  I want to to *not* have an st_mtime attribute,
> because it didn't have one before it was passed into
> _format_directory_entries, and _format_directory_entries should not
> modify its input.

Same comment as above. It reads to me that you don't care whether it has
an attribute, or if it's None, either is fine. Again, either a comment,
or using the 'missing' pattern would make things much easier to
understand.

Also, why do you have this assert as the last check? I'm asking, since
somehow it feels natural to me to first check the pre-conditions, and
then make sure the code works, rather then first showing that the code
works, and then check the pre-conditions. For example, what happens if
the pre-condition fails? Where is the error reported? Of course, there's
also the question, do you need to check that that the attribute is
missing? What happens if stat_result suddenly would gain that attribute?
Does _format_directory_entries() start to fail?


-- 
Björn Tillenius | https://launchpad.net/~bjornt