[rfc] [patch] small cleanups

[John Arbash Meinel]

Denys Duchier wrote:
> John Arbash Meinel <john at arbash-meinel.com> writes:
> 
> 
>>for func in (os.mkfifo, os.mknod, os.mkdev):
> 
> 
> hmm... that won't work if these functions don't exists (already a bug in my test
> catching just OSError - I knew I would forget something when going to non bare
> except).  How about this instead:
> 
> 
>     def test_hashcache_raise(self):
>         """check that hashcache can raise BzrError"""
>         from bzrlib.hashcache import HashCache
>         import os
> 
>         os.mkdir('.bzr')
>         hc = HashCache(u'.')
>         ok = False
> 
>         # make a best effort to create a weird kind of file
>         these = ((os,("mkfifo",),('a',)),
>                  (os,("mknod" ,),('a',)),
>                  (os,("mkdev" ,),('a',)))
>         
>         for mod, attrs, args in these:
>             x = mod
>             for a in attrs:
>                 if hasattr(x,a):
>                     x = getattr(x,a)
>                 else:
>                     x = None
>                     break
>             if x is None:
>                 continue
>             try:
>                 x(*args)
>                 ok=True
>             except OSError:
>                 pass
> 
>         from bzrlib.errors import BzrError
>         if ok:
>             self.assertRaises(BzrError, hc.get_sha1, 'a')
>         else:
>             raise BzrError("no weird file type could be created: extend this test case for your os")
> 
> --Denys

This seems overly complex for a simple test case.
Also, I would finish it with:
if ok:
...
else:
  self.fail("no weird file type...")
And you might consider doing:

else:
  raise TestSkipped("no weird file type ...")

It just depends how much of an error you consider it if we can't create
a weird file.

For something as simple as this test, I would just do:

for func_name in ('mkfifo', 'mknod', 'mkdev'):
  func = getattr(os, func_name, None)
  if func is None:
    continue
  try:
    func('a')
  except OSError:
    pass

That makes it more obvious what you are actually doing. Since for the
test you are passing the same thing to each function, and it is a member
of the same module.
Especially if someone has to come along and update this test in the
future, your full version is kind of hard to understand.

John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 256 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060111/733868e6/attachment.pgp