On Thu, Apr 28, 2016 at 01:07:25PM +0200, Klaus Aehlig wrote:
> On Wed, Apr 27, 2016 at 11:10:18PM +0100, 'Brian Foley' via ganeti-devel
> wrote:
> LGTM,
>
> just a few minor style issues.
>
> > -- | Computes the record name for a given field, based on either the
> > -- string value in the JSON serialisation or the custom named if any
> > -- exists.
> > fieldRecordName :: Field -> String
> > fieldRecordName (Field { fieldName = name, fieldConstr = alias }) =
> > - fromMaybe (camelCase name) alias
> > + case alias of
> > + Nothing -> camelCase . T.unpack $ name
> > + Just a -> T.unpack a
>
> why not
>
> maybe (camelCase $ T.unpack name) T.unpack alias
>
> ?
D'oh! Why not, indeed? Fixed.
> > -- | Construct a function that parses a field value. If the field has
> > @@ -324,7 +327,7 @@ parseFn :: Field -- ^ The field definition
> > -> Q Exp -- ^ The resulting function that parses a JSON message
> > parseFn field o =
> > let fnType = [t| JSON.JSValue -> JSON.Result $(fieldType field) |]
> > - expr = maybe [| readJSONWithDesc $(stringE $ fieldName field) |]
> > + expr = maybe [| readJSONWithDesc $(stringE $ T.unpack $ fieldName
> > field) |]
>
> prefer ... $(stringE . T.unpack $ fieldName field)
>
> (see
> http://docs.ganeti.org/ganeti/master/html/dev-codestyle.html#parentheses-point-free-style)
>
Ah rats, I thought I got all of these. Obviously I missed one. Fixed.
>
> Normally we don't do whitespace changes in untouched lines; here you have a
> big
> hunk of changed code, with the actual change being one " T.pack . " added.
Absolutely. Normally I wouldn't, but the indentation was much deeper than
elsewhere and the additional T.pack would have forced me to make that line
longer than 80 chars. A 'better' diff tool eg diff-so-fancy will highlight
the whitespace and content changes.
Thanks,
Brian.
