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

On Wed, Jun 1, 2011 at 3:01 PM, Gustavo Niemeyer <email address hidden> wrote:
> Review: Needs Fixing
> 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'/
>
>

Changed

> [12]
>
> +        config = ConfigOptions()
> +        content = None
>
> The second line may be dropped.
>
>

Fixed.

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

Done

>
> [14]
>
> +             "validator": String(),
>
> We need a new RegEx type in the schema to validate the regular
> expression.
>

Added with simple tests.

>
> [15]
>
> +            optional=["default", "description",]),
>
> s/",]/"]/
>
> Same on the bottom one.
>

Done

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

While I made these changes its worth noting that they are not exactly
the same as a missing config.yaml means the service takes no options.
So, for example, file not found isn't considered an error. This is to
simplify formula development, all services take no options now and
should continue to run that way. Once `formulate` becomes standard we
might require this file with an empty options dict as that will be
generated for the author.

> [17]
>
> +    def validate(self, options):
> +        """Validate options using the file at `path`.
>
> Which path?

Corrected ;)
>
>
> [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.
>
Done

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

I am using the property of () that allows for extending the statement
to the next line to work around the 80 char limit. In some cases with
later reformatting I was able to roll it back on to the previous line.
I don't love the way this looks but its an alternative to the char
limit and I never liked '\' continuations either. I'd rather we had a
solid commitment to turning off the 80 char limit in pep8 checker.

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

Changed, this changes point [22] below as well.

>
> [21]
>
>
> +class ServiceConfigError(Exception):
> +    """Indicates an issue related to service options."""
> +    pass
>
> Drop pass.

Done

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

regex was added after the base types which were all using the `value =
int(value)` pattern. Adding an additional argument meant it was in
keeping with the existing validation api. That said fixing the root
problem like you suggested in [20] was a much better fix.

>
>
> [23]
>
> +    "float": float,
> +    "regex": regex_validator
>
> Let's have proper validators, with an even interface on all of them,
> as per [20].
>

Done.

>
>
> Alright, I think with that we can merge it.
> --
> https://code.launchpad.net/~bcsaller/ensemble/config-set/+merge/60235
> You are the owner of lp:~bcsaller/ensemble/config-set.
>