Additional project niceties.

Additional project niceties.



https://code.launchpad.net/~benji/charm-tools/prooflib/+merge/197439

(do not edit description out of merge proposal)

[benji, 2013-12-03 14:40:00]

Comments for Marco.

https://codereview.appspot.com/36190043/diff/20001/.lbox
File .lbox (right):

https://codereview.appspot.com/36190043/diff/20001/.lbox#newcode1
.lbox:1: propose -cr -for lp:charmtools
Set the default options for "lbox propose".

https://codereview.appspot.com/36190043/diff/20001/.lbox.check
File .lbox.check (right):

https://codereview.appspot.com/36190043/diff/20001/.lbox.check#newcode1
.lbox.check:1: #!/bin/bash
This file is run pre-propose and pre-submit.  If it returns an error the
propose/submit is cancelled.

https://codereview.appspot.com/36190043/diff/20001/Makefile
File Makefile (right):

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode7
Makefile:7: #SHELL = $(warning [$@ ($^) ($?) ])$(OLD_SHELL)
This snippet is incredibly handy when debugging complex Makefile issues, but for
one this small it may not be worth maintaining here.

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode24
Makefile:24: virtualenv .
Using a virtualenv gives us (some) isolation from the system, enhancing build
reproducibility and reducing spurious errors.

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode34
Makefile:34: build: deps bin/python bin/pip develop bin/test
This target is the core of the Makefile.

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode36
Makefile:36: sysdeps:
This installs the (few) system dependencies.

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode122
Makefile:122: .DEFAULT_GOAL := build
A bare "make" command should build the software.

https://codereview.appspot.com/36190043/diff/20001/charmtools/__init__.py
File charmtools/__init__.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/__init__.py#old...
charmtools/__init__.py:19: import glob
Lint: unused import.

https://codereview.appspot.com/36190043/diff/20001/charmtools/bundles.py
File charmtools/bundles.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/bundles.py#oldc...
charmtools/bundles.py:2: import sys
More lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/charms.py
File charmtools/charms.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/charms.py#oldcode2
charmtools/charms.py:2: import sys
Lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/cli.py
File charmtools/cli.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/cli.py#oldcode4
charmtools/cli.py:4: import argparse
So much lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/mr.py
File charmtools/mr.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/mr.py#oldcode19
charmtools/mr.py:19: from bzrlib import errors, trace
Lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/promulgate.py
File charmtools/promulgate.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/promulgate.py#o...
charmtools/promulgate.py:27: import string
Lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/promulgate.py#o...
charmtools/promulgate.py:32: from bzrlib.plugins.launchpad import lp_api
Lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/search.py
File charmtools/search.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/search.py#oldco...
charmtools/search.py:19: import re
Lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/subscribers.py
File charmtools/subscribers.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/subscribers.py#...
charmtools/subscribers.py:126: charm =
charmdistro.getSourcePackage(name=charm_name)
So... the linter complained that "charm" was assigned a value but never used,
which is true.  Looking at the code I saw that "charm_name" was appended to the
list "charms" so I figured s/charm_name/charm/ was the right thing to do, so I
did it and ran the tests.  None failed.  I ran "make check" and some failed.  I
changed the code back and "make check" still failed.  I ran "make check" on
lp:charm-tools/1.2 and it failed in the same way there.

I looked at the other code and despite the list being named "charms", charm
names are appended to it below, so I really don't know what to do here.

Therefore, just removed the assignment to make the linter happy and left it for
you to figure out. :)

https://codereview.appspot.com/36190043/diff/20001/charmtools/tests/test_proo...
File charmtools/tests/test_proof.py (right):

https://codereview.appspot.com/36190043/diff/20001/charmtools/tests/test_proo...
charmtools/tests/test_proof.py:36: self.linter = CharmLinter()
This makes the tests pass, they fail on both lp:charm-tools and
lp:charm-tools/1.2.

https://codereview.appspot.com/36190043/diff/20001/charmtools/update.py
File charmtools/update.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/update.py#oldco...
charmtools/update.py:55: print >> sys.stderr,  ".mrconfig not saved: ", e
Lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/update.py
File charmtools/update.py (right):

https://codereview.appspot.com/36190043/diff/20001/charmtools/update.py#newco...
charmtools/update.py:55: print >> sys.stderr, ".mrconfig not saved: ", e
I bet you also want to remove the last space in the string, otherwise there will
be two spaces between the colon and the error.

Also, I highly recommend you use "from __future__ import print_function" which
makes print into a function instead of a keyword.  This is both
forward-compatible with Python 3 and nicer to use.  If you did that here the
above line would change to:

    print(".mrconfig not saved:", e, file=sys.stderr)

https://codereview.appspot.com/36190043/diff/20001/scripts/test
File scripts/test (right):

https://codereview.appspot.com/36190043/diff/20001/scripts/test#newcode7
scripts/test:7: bin/nosetests --exe --with-id $extra_args $@
The "--exe" option is required because the tests file is executable.  This is
likely an artifact of not having a test runner previously.  I suggest removing
the __main__ test in the test file and removing the executable bit.

https://codereview.appspot.com/36190043/diff/20001/setup.py
File setup.py (left):

https://codereview.appspot.com/36190043/diff/20001/setup.py#oldcode13
setup.py:13: from setuptools import setup, find_packages
Lint.

[Marco Ceppi, 2013-12-03 19:51:14]

These changes look fine, I'm not sure if .lbox is configured properly or not.
Could you review?

https://codereview.appspot.com/36190043/diff/20001/.lbox
File .lbox (right):

https://codereview.appspot.com/36190043/diff/20001/.lbox#newcode1
.lbox:1: propose -cr -for lp:charmtools
On 2013/12/03 14:40:01, benji wrote:
> Set the default options for "lbox propose".

lp:charm-tools

https://codereview.appspot.com/36190043/diff/20001/charmtools/subscribers.py
File charmtools/subscribers.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/subscribers.py#...
charmtools/subscribers.py:126: charm =
charmdistro.getSourcePackage(name=charm_name)
On 2013/12/03 14:40:01, benji wrote:
> So... the linter complained that "charm" was assigned a value but never used,
> which is true.  Looking at the code I saw that "charm_name" was appended to
the
> list "charms" so I figured s/charm_name/charm/ was the right thing to do, so I
> did it and ran the tests.  None failed.  I ran "make check" and some failed. 
I
> changed the code back and "make check" still failed.  I ran "make check" on
> lp:charm-tools/1.2 and it failed in the same way there.
> 
> I looked at the other code and despite the list being named "charms", charm
> names are appended to it below, so I really don't know what to do here.
> 
> Therefore, just removed the assignment to make the linter happy and left it
for
> you to figure out. :)

Yeah, I didn't write this :) I'll go over it post merge and review what it's
trying to do.

[benji, 2013-12-04 14:00:17]

Fixed typo.

https://codereview.appspot.com/36190043/diff/20001/.lbox
File .lbox (right):

https://codereview.appspot.com/36190043/diff/20001/.lbox#newcode1
.lbox:1: propose -cr -for lp:charmtools
On 2013/12/03 19:51:14, Marco Ceppi wrote:
> On 2013/12/03 14:40:01, benji wrote:
> > Set the default options for "lbox propose".
> 
> lp:charm-tools

Done.

[benji, 2013-12-04 15:38:15]

Please take a look.

[Marco Ceppi, 2013-12-05 16:30:50]

https://codereview.appspot.com/36190043/diff/40001/Makefile
File Makefile (right):

https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode42
Makefile:42: bzr checkout lp:~juju-jitsu/charm-tools/dependencies
Can we not have it owned by ~juju-jitsu and instead place it under ~charmers?

https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode47
Makefile:47: $(PYTHON_PACKAGE_CANARY): requirements.txt | bin/pip dependencies
Why wouldn't we keep requirements.txt for deps? Decoupling them means they could
change between versions of charm-tools and the dependencies branch. Having the
requirements.txt be the source for deps makes more sense to me. Unless I'm
missing the point of this line.

[benji, 2013-12-05 16:41:16]

Here are my responses to the most recent round of review notes.

https://codereview.appspot.com/36190043/diff/40001/Makefile
File Makefile (right):

https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode42
Makefile:42: bzr checkout lp:~juju-jitsu/charm-tools/dependencies
On 2013/12/05 16:30:50, Marco Ceppi wrote:
> Can we not have it owned by ~juju-jitsu and instead place it under ~charmers?

Sure.  Is lp:~charmers/charm-tools/dependencies an acceptable/valid location?

https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode47
Makefile:47: $(PYTHON_PACKAGE_CANARY): requirements.txt | bin/pip dependencies
On 2013/12/05 16:30:50, Marco Ceppi wrote:
> Why wouldn't we keep requirements.txt for deps? Decoupling them means they
could
> change between versions of charm-tools and the dependencies branch. Having the
> requirements.txt be the source for deps makes more sense to me. Unless I'm
> missing the point of this line.

The requirements.txt file specifies the dependencies.  The dependencies
directory exists so all the dependencies are conveniently available in one place
and to make sure the dependencies are available (people have a bad habit of
removing "old" distributions from the web) wile also removing the need for web
access to build the software (building on a plane or behind an egress-filtering
firewall are important use cases).

[Marco Ceppi, 2013-12-05 16:43:44]

On 2013/12/05 16:41:16, benji wrote:
> Here are my responses to the most recent round of review notes.
> 
> https://codereview.appspot.com/36190043/diff/40001/Makefile
> File Makefile (right):
> 
> https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode42
> Makefile:42: bzr checkout lp:~juju-jitsu/charm-tools/dependencies
> On 2013/12/05 16:30:50, Marco Ceppi wrote:
> > Can we not have it owned by ~juju-jitsu and instead place it under
~charmers?
> 
> Sure.  Is lp:~charmers/charm-tools/dependencies an acceptable/valid location?

This is fine, thanks!

> https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode47
> Makefile:47: $(PYTHON_PACKAGE_CANARY): requirements.txt | bin/pip dependencies
> On 2013/12/05 16:30:50, Marco Ceppi wrote:
> > Why wouldn't we keep requirements.txt for deps? Decoupling them means they
> could
> > change between versions of charm-tools and the dependencies branch. Having
the
> > requirements.txt be the source for deps makes more sense to me. Unless I'm
> > missing the point of this line.
> 
> The requirements.txt file specifies the dependencies.  The dependencies
> directory exists so all the dependencies are conveniently available in one
place
> and to make sure the dependencies are available (people have a bad habit of
> removing "old" distributions from the web) wile also removing the need for web
> access to build the software (building on a plane or behind an
egress-filtering
> firewall are important use cases).

I appreciate the explanation. So as we add deps/move to other versions we'd just
include those in addition to the older versions? Either way I approve of this.

LGTM!