Code review comment for lp:~jimbaker/pyjuju/charm-format-2

Please take a look at the latest proposed version of this branch, this
should solve the issues that were raised.

http://codereview.appspot.com/6308044/diff/10001/juju/charm/metadata.py
File juju/charm/metadata.py (right):

http://codereview.appspot.com/6308044/diff/10001/juju/charm/metadata.py#newcode239
juju/charm/metadata.py:239: if self.format not in (PYTHON_FORMAT,
YAML_FORMAT):
On 2012/06/12 21:18:08, hazmat wrote:
> This seems to be conflating two different things, charm api versions
and there
> corresponding formats. ie. we could add version 3, with the existing
yaml
> format. there should be a mapping of format to version. the charm
isn't making a
> request for format, its making a version api assertion.

I changed this so there's a definite distinction, through the use of
Format1 and Format2 classes (no more PYTHON_FORMAT, YAML_FORMAT). Assume
at some point we have a format: 3 and it's supported by the Python code,
then we could readily create a Format3 class that either derives from
Format2, or from BaseFormat, or what makes sense. So this separates the
charm version information (and any assertions) from the implementation.

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/protocol.py
File juju/hooks/protocol.py (right):

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/protocol.py#newcode220
juju/hooks/protocol.py:220: if invoker.charm_format == PYTHON_FORMAT:
The code has been changed such that one can get an appropriate formatter
(Format1, Format2 for now) and do the appropriate action, without the
conditional logic being replicated throughout as you've observed.

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/utils.py
File juju/hooks/utils.py (right):

http://codereview.appspot.com/6308044/diff/10001/juju/hooks/utils.py#newcode10
juju/hooks/utils.py:10: env_charm_format =
os.environ.get("_JUJU_CHARM_FORMAT")
I have changed this usage where feasible to what you describe, however,
in cases like relation_set, it's necessary to do the parsing on the
client side before calling the server side of the implementation. In
this case, it's still necessary to define _JUJU_CHARM_FORMAT. However,
this lookup is now more isolated, because it's been moved to
juju.lib.format.

http://codereview.appspot.com/6308044/