Code review comment for lp:~rvb/maas/bug-1287310

> Looks good, nice improvement.

Thanks for the review!

> [1]
>
> 146     +    return "'%s' is not a valid %s.  It should be one of: %s." % (
> 147     +        "%(value)s",
> 148     +        choice_of_what,
> 149     +        ", ".join("'%s'" % name for name, value in valid_choices),
> 150     +    )
>
> Consider using englist_list() from Launchpad to produce an even better
> string of choices. I suspect Django has something similar though.

Not that I can find right now. I'll keep it like this for now… we can always improve it later.

> [2]
>
> 148     +        choice_of_what,
> 149     +        ", ".join("'%s'" % name for name, value in valid_choices),
> 150     +    )
>
> Is valid_choices sorted?

No, but it's a list of tuples so it's better to just keep the ordering of that list.

> [3]
>
> 146     +    return "'%s' is not a valid %s.  It should be one of: %s." % (
> 147     +        "%(value)s",
>
> Or double-up the percent symbol:

Hum, didn't know that trick!

> [4]
>
> 182     +        choices = [
> 183     +            (factory.make_name('value'), factory.make_name('key'))
> 184     +            for _ in range(2)]
>
> I think value and key are the wrong way round here.

Well spotted, fixed.