Re: Make define-builtin-markup{, -list}-command #:category #:properties keywords (issue160048)

[David Kastrup]

address@hidden writes:

> David,
>
> Thank you for posting this on Rietveld.  It was much easier for me to
> review it.
>
> I have one documentation question (see below).
>
> I also have one patch philosophy question: should define-markup also
> have (at least) a :properties keyword added to it?

Goal #1 was to be able to move user-level markup to system markup
unchanged.

This goal actually is not achieved with the current state of the patch
series since it fails for markup definitions without DOC string.

> I think that if it did, then we'd have the possibility of using the
> exact same code and just changing from define-markup to
> define-builtin-markup by only changing the macro name.

After fixing the DOC string issue and exporting
define-builtin-markup-command as define-markup-command, the regression
test seems to work fine (namely, it runs until it crashes on "Unable to
load the map file" which it always does here) and the scoping would
appear to be correct as well.

Which suggests that the apparent main reason for *-builtin-* commands,
the changed toplevel scoping, is not valid, and the scoping works out
well enough without extra complications.

Renaming define-builtin-markup-command to define-markup-command, making
it public and adapting all callers would appear to finish the job,
obviously unifying the syntax.

I have to check that this is indeed the case.

> http://codereview.appspot.com/160048/diff/1/5
> File scm/markup.scm (right):
>
> http://codereview.appspot.com/160048/diff/1/5#newcode74
> scm/markup.scm:74: [ :category category ]
> Does this need to be [ #:category category ] ?

Yes.  Sorry.

-- 
David Kastrup