[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [ELPA] Update proposal: psgml: asl-like parsing in PI
From: |
Stefan Monnier |
Subject: |
Re: [ELPA] Update proposal: psgml: asl-like parsing in PI |
Date: |
Wed, 22 Nov 2017 17:15:04 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) |
Hi,
[ I Cc'd Lennart, but I don't think he reads this email address any more,
and AFAIK he's not interested in maintaining PSGML any more.
If you're interested, you could be the new maintainer. ]
> Instead of operating immediately on the values it returns a list of strings
> and (name . value) pairs that the handler function can operate on instead,
> with sgml--pi-element-handler as an example.
Sounds good.
> See the attached patch file for specifics. I'd appreciate some feedback on
> the docstring and the possibility of turning on and off floating literals as
> an optional argument so it could be used as the parser for
> sgml-parse-attribute-specification-list as well.
Not sure what you mean by "turning on and off floating literals".
[ I've taken charge of including psgml into GNU ELPA and cleanup the
code for use in Emacs-24, but I'm generally not familiar with the code
and I've never used PSGML myself. ]
> -(defun sgml--pi-element-handler ()
> +(defun sgml-parse-name-or-literal ()
> (sgml-parse-s)
> - (let ((eltype (sgml-lookup-eltype (sgml-parse-name)))
> - name value)
> + (if (looking-at "['\"]")
> + (sgml-parse-literal)
> + (sgml-parse-name)))
AFAICT the old code used (read (current-buffer)) instead of
(sgml-parse-name) here. What's the implication of this change?
> +(defun sgml-pi-asl-parser (&optional value-fn)
I'd personally just make `value-fn` into a mandatory argument.
[ Which also justifies using the name `sgml-parse-name-or-literal`
without double dash, since that function might be useful for external
callers to provide as value-fn argument.
But sgml-parse-name-or-literal probably needs a docstring then. ]
> + "Parse the processing instruction like an attribute specification list.
> +
> +Returns a list of strings and (name-string . value-string) pairs
> +in the order they were written in; atom strings when there is no
> +value indicator and conses when there is.
We use all-caps names as metavariables in docstrings. So I'd write it
something like:
Return a list of elements of the form (NAME . VALUE) or just NAME
when there was no value indicator.
> +As an extension, the atom string can appear as a literal instead
> +of only a token.
I don't know what that means.
> +As an extension, the value of a name-value pair can be parsed by
> +calling value-fn with no arguments instead of as a token or
> +literal."
Usually we use the following wording:
Optional argument VALUE-FN when provided is called with no argument
to parse the value part of a name-value pair, instead of parsing it
as a token or a literal. It should return the VALUE represented as
a string.
> (cond ((sgml-parse-delim "VI")
> + (setq val (funcall (if value-fn value-fn
> 'sgml-parse-name-or-literal))))
> + (t (setq val nil)))
(if A A B) is better written (or A B):
(cond ((sgml-parse-delim "VI")
(setq val (funcall (or value-fn
#'sgml-parse-name-or-literal))))
(t (setq val nil)))
And the `setq val` can be hoisted outside the conditional:
(setq val (cond ((sgml-parse-delim "VI")
(funcall (or value-fn #'sgml-parse-name-or-literal)))
(t nil)))
And the trailing (t nil) can be dropped:
(setq val (if (sgml-parse-delim "VI")
(funcall (or value-fn #'sgml-parse-name-or-literal))))
Some people may prefer `when` over `if` here.
Also, rather than let-bind `val` outside the loop and `setq` it at each
turn, we can do:
(let ((val (if (sgml-parse-delim "VI")
(funcall (or value-fn #'sgml-parse-name-or-literal)))))
(push (if (null value) name (cons name value)) acc))
With the use of `lexical-binding` this form is just as efficient.
> + (push (if (null val) name (cons name val)) acc)
AFAICT the old code used `t` rather than `nil` to mark the "no
value" case. Is there a risk that sgml-parse-name-or-literal can
return nil? If so, is the resulting change in behavior on purpose?
If it can't return nil, then the previous code can be simplified further
to have a single `if` test.
> +(defun sgml--pi-element-handler ()
> + (destructuring-bind (elname &rest options)
the new psgml-parse.el code uses `cl-lib` rather than `cl` so the above
needs to be changed to `cl-destructuring-bind`.
Stefan