upgradeprovision-review comments

[Jelmer Vernooij]

Hi Matthieu,

Overall, I would be happy to see your upgradeprovision-review branch
land but I have a couple of comments:

 * please don't reintroduce cmdline_loadparm but use
samba.tests.env_loadparm() instead. I'm trying to get rid of subunitrun
and instead allow any sort of python test runner to run Samba python
tests
 * in create_dummy_secretsdb() don't catch all exceptions and return
None if there was one. Since this function is used in the testsuite only
we really should be raising the exception so we can fix it. I also don't
expect any of the callers to handle None rather than a LDB connection
being returned
 * the grouped transactions need to cope with exceptions during
commit/cancel better (see IRC conversation)
 * the upgradeprovisionneeddc test tries to load two different smb.conf
files, it should stick with one.

These should be fixed at some point but I'm okay with landing this
branch and addressing them later:

 * in the tests, please use assertEquals(a, b) rather than assertTrue(a
== b) since the first will make the test runner print a more
comprehensible error message including the values of a and b
 * there is no need to catch exceptions that you are not expecting in
the testsuite. the test runner will catch them for you and display them.
Code like this will just hide the real error, requiring you to put in a
print statement to figure out what is really going on:
  try:
    foo
  except:
    self.assertTrue(0)
 
 * Use the constants (True, False) for booleans rather than (0, 1)

 * There is no need for a terminating backslash when continuing a line
when you're inside of parentheses

 * Nitpicky style issues (PEP8):
   * Use "is None" and "is not None" rather than "== None" and "!= None"
   * The closing """ for multi-line docstrings should be on its own line
   * There should be two empty lines between top-level objects
(functions, classes, etc)
   * avoid camelcasing, our style for variable names is to use
underscores (smb_conf_path rather than smbConfPath)
   * rather than 'if 0: print "none"', use "pass"

 * os.path.join() accepts more than two arguments, there is no need to
nest calls to it

Cheers,

Jelmer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100615/71a60b36/attachment.pgp>