Re: [ELPA] Update proposal: psgml: asl-like parsing in PI

[Stefan Monnier]

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