Hi Maxim, On Fri, May 07 2021, Maxim Cournoyer wrote:
> Hello Xinglu! > > Thank you for working on it! You are very welcome! These are things that have annoyed me enough so I decided (try) to fix it myself :) >> +(define (configuration-no-default-value kind field) >> + (configuration-error >> + (format #f "`~a' in `~a' does not have a default value" kind field))) > > The kind and field should be inverted. Good catch >> configuration-field make-configuration-field configuration-field? >> @@ -112,7 +116,7 @@ >> (define-syntax define-configuration >> (lambda (stx) >> (syntax-case stx () >> - ((_ stem (field (field-type def) doc) ...) >> + ((_ stem (field (field-type properties ...) doc) ...) > > I'd rather keep the 'def' binding for the default value; properties is > too vague and implies many of them, which is not a supported syntax. Yeah, not exactly sure why I changed it to ‘properties’, anyway... >> (with-syntax (((field-getter ...) >> (map (lambda (field) >> (id #'stem #'stem #'- field)) >> @@ -121,36 +125,56 @@ >> (map (lambda (type) >> (id #'stem type #'?)) >> #'(field-type ...))) >> + ((field-default ...) >> + (map (match-lambda >> + ((field-type default _ ...) default) >> + ;; We get warnings about `disabled' being an >> + ;; unbound variable unless we quote it. >> + (_ (syntax 'disabled))) > > Here I think it'd be better to have the pattern more strict (e.g, > (field-type default-value) or (field-type); so as to not accept invalid > syntax. Good point! > I also think it'd be clearer to use another symbol than 'disabled, as > this already has a meaning for the validator and could confuse readers. Yeah, it’s not very descriptive. >> + #'((field-type properties ...) ...))) >> ((field-serializer ...) >> (map (lambda (type) >> (id #'stem #'serialize- type)) >> #'(field-type ...)))) >> - #`(begin >> - (define-record-type* #,(id #'stem #'< #'stem #'>) >> - #,(id #'stem #'% #'stem) >> - #,(id #'stem #'make- #'stem) >> - #,(id #'stem #'stem #'?) >> - (%location #,(id #'stem #'-location) >> - (default (and=> (current-source-location) >> - source-properties->location)) >> - (innate)) >> - (field field-getter (default def)) >> - ...) >> - (define #,(id #'stem #'stem #'-fields) >> - (list (configuration-field >> - (name 'field) >> - (type 'field-type) >> - (getter field-getter) >> - (predicate field-predicate) >> - (serializer field-serializer) >> - (default-value-thunk (lambda () def)) >> - (documentation doc)) >> - ...)) >> - (define-syntax-rule (stem arg (... ...)) >> - (let ((conf (#,(id #'stem #'% #'stem) arg (... ...)))) >> - (validate-configuration conf >> - #,(id #'stem #'stem #'-fields)) >> - conf)))))))) >> + #`(begin >> + (define-record-type* #,(id #'stem #'< #'stem #'>) >> + #,(id #'stem #'% #'stem) >> + #,(id #'stem #'make- #'stem) >> + #,(id #'stem #'stem #'?) >> + (%location #,(id #'stem #'-location) >> + (default (and=> (current-source-location) >> + source-properties->location)) >> + (innate)) >> + #,@(map (lambda (name getter def) >> + (if (equal? (syntax->datum def) (quote 'disabled)) > > nitpick: eq? suffices to check for symbols. > >> + #`(#,name #,getter) >> + #`(#,name #,getter (default #,def)))) >> + #'(field ...) >> + #'(field-getter ...) >> + #'(field-default ...))) >> + (define #,(id #'stem #'stem #'-fields) >> + (list (configuration-field >> + (name 'field) >> + (type 'field-type) >> + (getter field-getter) >> + (predicate field-predicate) >> + (serializer field-serializer) >> + ;; TODO: What if there is no default value? > > Seems this TODO was taken care of already :-). > >> + (default-value-thunk >> + (lambda () >> + (display '#,(id #'stem #'% #'stem)) >> + (if (equal? (syntax->datum field-default) >> + (quote 'disabled)) > > Like above (eq? would do). More importantly (and confusingly), here the > 'disabled expected value must *not* be quoted. I haven't investigated > why but it seems one level of quote got striped at that point. Hmm, I am still a little confused about how quotation works with syntax-case & friends. >> + (configuration-no-default-value >> + '#,(id #'stem #'% #'stem) 'field) >> + field-default))) >> + (documentation doc)) >> + ...)) >> + (define-syntax-rule (stem arg (... ...)) >> + (let ((conf (#,(id #'stem #'% #'stem) arg (... ...)))) >> + (validate-configuration conf >> + #,(id #'stem #'stem #'-fields)) >> + conf)))))))) > > The following patch implements the above comments; > > [...] > > I'll attempt to review patch 2/2 shortly! > > Thanks a lot for this neat improvement! Thank you for the review and the improvements!