bug#24389: [PATCH] Support completion of classes and IDs in CSS mode

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

> What do you think?

I like the general idea, but have some nitpicks below.

> +(defun css--complete-html-tag ()
> +  "Complete HTML tag at point."
> +  (save-excursion
> +    (let ((end (point)))
> +      (skip-chars-backward "-[:alnum:]")
> +      (list (point) end css--html-tags))))

I don't see where this is used.

> +(defvar css--foreign-completion-cache
> +  (list 'html-class-extractor-function (make-hash-table :test 'equal)
> +        'html-id-extractor-function (make-hash-table :test 'equal))

I urge you to use alists rather than plist whenever you can.  I only
recommend plists for those rare cases where they're likely to often be
written down by end-users who'd be bothered by the need to add dots
and parentheses.

> +(defun css--foreign-completions (extractor)
> +  "Return a list of completions provided by other buffers.
> +EXTRACTOR should be the name of a function that may be defined in
> +one or more buffers.  In each of the buffers where EXTRACTOR is
> +defined, EXTRACTOR is called and the results are accumulated into
> +a list."
> +  (seq-uniq
> +   (seq-mapcat
> +    (lambda (buf)
> +      (with-current-buffer buf
> +        (when (boundp extractor)
> +          (let ((cache
> +                 (plist-get css--foreign-completion-cache extractor)))
> +            (if cache
> +                (let ((hash (buffer-hash buf)))
> +                  (or (gethash hash cache)
> +                      (puthash hash (funcall (symbol-value extractor))
> +                               cache)))
> +              (funcall (symbol-value extractor)))))))
> +    (buffer-list))))

I think this design won't work well.  How 'bout instead just always
calling the function and let the other side handle the caching?

This way the cache can work correctly even in case the list of
completions depends on sources other than the buffer's content (a case
which even your costly buffer-hash won't catch).  Also, it means that
depending on the cost of the extraction itself, the cache may be flushed
more or less eagerly.

> +(defvar html-class-extractor-function)
> +(defvar html-id-extractor-function)

The definition of those global variables seems to be missing.
IIUC they should be in css-mode.el, so their name should start with
"css-" I think (tho I agree that it's debatable).

In any case I think it's important that their global value be a valid
function rather than nil (it should probably be #'ignore).

Also, if the caching is moved to those functions, then their name should
not say "extractor" but should simply indicate that those functions
should return the list of classes/ids.  E.g. css-class-list-function.


        Stefan