Code review comment for lp:~gmb/maas/json-schema-part-1

On Friday 07 Feb 2014 09:20:37 you wrote:
> Commit message:
> Convert the definition of power type parameters from straight Python to
> something that's JSON-schema-validatable.

Can you add something about why this is happening please.

> After a lot of dancing around, this is the first part of the work to making
> power type parameters specifiable in JSON.

\o/

> In order to do this I've:
>
> - Defined a JSON schema to describe how power type parameters should look
> (this is split up so that we can validate separate parts of it
> individually) - Created a JSON-validatable version of the existing
> POWER_TYPE_PARAMETERS dict - Created a function,
> get_power_type_parameters_from_json() to convert the JSON power type
> parameters to something similar to the existing POWER_TYPE_PARAMETERS dict.
> - Use get_power_type_parameters_from_json() to create
> POWER_TYPE_PARAMETERS. This is done once at the bottom of
> maasserver.power_parameters so that we're not parsing and validating JSON
> all the time.

This is mostly looking *great*. I'd like to get Gavin to take a peek to make
sure it matches his grand vision, but it looks like it's on the right track to
me.

Some nits:

[1] You're missing :param:, :type: and :return: info on the docstrings for the
new functions.

(FWIW My vim plugin reads the :type: and offers better auto-completions :)

[2] I realise it's early days, but the FIELD_TYPE_MAPPINGS are going to form
part of the API with the drivers. I think putting these in their own module
would be better (like errno), so we can do something like
powerfield.mac_address, for example. This also then forms a firm contract
with the driver, which consumes the API. (It also makes it easier to install
for packaging and is something we should have done for enums so that maas api
clients can use them)

[3] jsonschema. MY EYES!
OK this ones not a nit :)

[4] Missing tests for make_json_field.

cheers.