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

On 10 February 2014 02:24, Julian Edwards <email address hidden> wrote:
> 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.

Yeah, sorry. I've gotten into a laziness habit with commit messages...

>> After a lot of dancing around, this is the first part of the work to making
>> power type parameters specifiable in JSON.
>
> \o/
>
> 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.

+1. Will bug Gavin.

> 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 :)

Which vim plugin loveliness is it of which you speak? Fixed.

>
> [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)

Okay, I'm a bit lost as to what you mean here... I was with you up
until "their own module" but then you started talking about packaging
and I got confused. Are you talking about just adding a module under
src/maasserver (which is what I think you mean)?

Anyway, in the interests of getting this landed I'll do that in a
followup branch once I've got clarity.

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

DEAR GOD YES.

> [4] Missing tests for make_json_field.
>

Fixed. Cheers for the review.