Code review comment for lp:~bcsaller/pyjuju/config-set

Idea looks good, implementation looks good, several small issues
in terms of polishing the branch for merging.

[11]

+description and an optional default value. Additionally type may be

s/type/'type'/

[12]

+ config = ConfigOptions()
+ content = None

The second line may be dropped.

[13]

+ "options": Dict(String(),
+ OneOf(
+ # regex type requires validator
+ KeyDict({"type": Constant("regex"),

Indentation should be revised here. Feel free to define an
auxiliary variable externally.

[14]

+ "validator": String(),

We need a new RegEx type in the schema to validate the regular
expression.

[15]

+ optional=["default", "description",]),

s/",]/"]/

Same on the bottom one.

[16]

+ @classmethod
+ def load_file(cls, pathname, optional=False):

Drop the @classmethod here. load() doesn't have it, and MetaData doesn't use it
either, so let's make the interface even. Also, please rename the functions to
conform to the same conventions established by MetaData: parse() and load(), the
former with a data _string_, the latter with a filename. Add parse_serialization_data
to handle the dict case. The behavior of these functions should also mimic MetaData:
load() raises FileNotFound, and so on.

These two classes are exactly alike. There's no reason for us to diverge and
then having to remind of which interface does what, which load accepts a dict,
etc.

[17]

+ def validate(self, options):
+ """Validate options using the file at `path`.

Which path?

[18]

+ This method will return a dict with options that have passed
+ validation. This means options not declared in the formula
+ metadata will trigger a ServiceConfigError exception.
+
+ This method includes defaults for options which have no values
+ supplied if such defaults are defined in the metadata.

Please reword:

This method validates all the provided options, and returns a new
dictionary with values properly typed and default values set for
missing keys. If a provided option is unknown or its value
fails validation, ServiceConfigError is raised.

[19]

+ "%s is not a valid configuration option." % (option))
(...)
+ "a normal Python regex pattern for %s" % (
+ option))
(...)
+ "See http://docs.python.org/library/re.html" % (
+ option))
(...)
+ raise ServiceConfigError("Invalid options specification: %s" % (
+ error))

This is a tuple: (option,)

This is an expression with redundant parenthesis: (option)

Drop the parenthesis in all of those cases.

[20]

+ validator = validation_kinds[kind]
+ if kind == "regex":
+ pattern = self._data[option].get("validator")

The interface for validators can deal with such distinctions so that
we don't have to special case them in the generic function. E.g.:

validator = validation_kinds[kind]
value, valid = validator(value, self._data[option])

[21]

+class ServiceConfigError(Exception):
+ """Indicates an issue related to service options."""
+ pass

Drop pass.

[22]

+def regex_validator(str, pat):
+ return re.match(pat, str) is not None

Don't your eyes bleed and pop with the obvious inversion of parameters?
I can't emphasise enough how important consistency is for the long term
health of a software project. It's _key_ to pay attention to details
such as parameter consistency, interface consistency across similar
classes, and so on. Those are the details that will make our lives
(or someone else's) less painful when giving maintenance in years to
come.

[23]

+ "float": float,
+ "regex": regex_validator

Let's have proper validators, with an even interface on all of them,
as per [20].

Alright, I think with that we can merge it.