Code review comment for lp:~allenap/maas/repackage

Revision history for this message
Gavin Panella (allenap) wrote :

> Reading a bit further. Getting a bit lost with all the repetition! Is there
> really no way to share common definitions of __version__, envvar,
> check_settings, and so on? It's a lot of stuff to have both duplicated and
> untested, even if it is "meta" code.

Bugger, I didn't mention that in the description. That's one of the
reasons I wanted to do an interactive review. The setup.py files are all
auto-generated from the call to common.configure().

I tried using pprint to make the call to setup() format better, but that
has its own problems. Ideally I'd like to get rid of that hack, but I
did try several other ways to do it. I was trying to make it so that
each package would be installable from PyPI, for example, where the
common package would no longer be available, hence the need for bundling
things into setup.py.

>
> .
>
> I can see how it could be useful in the future for envvar to yield the
> environment variable's original value. But it's also a potential source of
> misunderstanding: "with environment variable set to X, its value is Y." I
> would just leave that feature out until we actually need it.

Done.

>
> .
>
> Much of the duplicated code needs documentation, and I can't help feeling that
> the duplication is the reason for not documenting it — which would be a poor
> state of affairs. I'm thinking of check_settings in particular: which
> settings does this check, for whose benefit, to serve what need?

I've added docstrings to pkg/common/__init__.py, which now means that
all the setup.pys are also documented.

>
> .
>
> Why doesn't install_deps follow our naming conventions?

It follows the distutils/setuptools naming convention for commands. I
don't know why it's that way :-/

>
> .
>
> I think the linter needs an update to check the pkg directory. The setup()
> calls are on insanely long lines.

pkg/common/__init__.py is lint-free, and there's not much I can do about
those setup() calls right now.

>
> .
>
> Does pkg/common/checkarchive.py really compare contents of a tarball to a
> directory? Or only the file and directory names?

Just names. It was useful when figuring out why data_files and
include_package_data were not doing as I expected (see check_settings).
I've updated its docstring.

>
> .
>
> In pkg/maas-cluster/provisioningserver/testing/__init__.py you define "here"
> and "root" as global variables. Better upper-case them, as we have mostly
> been doing. Moreover "root" needs a more descriptive name and a bit of
> documentation. Root of what? And in a branch, in a package installation, or
> both?

I totally want to get rid of those! I'll see if I can.

« Back to merge proposal