Launchpad itself

Code review comment for lp:~benji/launchpad/wadl-refactoring

  1. wadl-refactoring
  2. Merge into devel
Revision history for this message
Gavin Panella (allenap) wrote :

Nice clean up :)

I have a few fairly minor comments. [4] needs fixing before this
branch is safe to land.

[1]

+ self.assert_(wadl.startswith('<?xml '))

I think this might be better as:

   from lp.testing.matchers import Startswith
   self.assertThat(wadl, StartsWith('<?xml '))

You'll need to inherit from lp.testing.TestCase (or, more accurately,
testtools.TestCase) to get assertThat().

[2]

+ self.assert_('<html ' in html)

Same as [1].

[3]

+ return subprocess.Popen(
+ ['xsltproc', stylesheet, wadl_filename],
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE).communicate()[0]

Unless it's useless and/or noisy, I think stderr should be left alone,
so:

    return subprocess.Popen(
        ['xsltproc', stylesheet, wadl_filename],
        stdout=subprocess.PIPE).communicate()[0]

I also think the exit code should be checked, so:

    args = ['xsltproc', stylesheet, wadl_filename]
    process = subprocess.Popen(args, stdout=subprocess.PIPE)
    out, err = process.communicate()
    if process.returncode == 0:
        return out
    else
        raise subprocess.CalledProcessError(
            process.returncode, args)

This is similar to how subprocess.check_call() behaves.

[4]

-import _pythonpath
-

It's ugly, looks like lint, but you'll need this.

It may be worth adding a test to show that the script can run
successfully, even if it's only to show the help.

review: Needs Fixing (code)

« Back to merge proposal