Re: changes to cfengine-mode

[Stefan Monnier <monnier@iro.umontreal.ca>]

> OK.  See attached, and I hope final, patch.

A few more nit picks, feel free to install the patch after addressing them.

> +(defcustom cfengine-mode-debug nil
> +  "Whether `cfengine-mode' should print debugging info
> + (only used by the cfengine3 support currently)"
> +  :group 'cfengine
> +  :type 'boolean)

We usually make those debug variables with `defvar' rather than `defcustom'.

>  (defcustom cfengine-mode-abbrevs nil
> -  "Abbrevs for Cfengine mode."
> +  "Abbrevs for CFEngine2 mode.
> +Obsolete, see `edit-abbrevs' instead."
[...]
> +(make-obsolete-variable 'cfengine-mode-abbrevs nil "24.1")

Don't mention "Obsolete" in the docstring, since the
make-obsolete-variable already gives this info.  And please put the
"use edit-abbrevs" as second argument to make-obsolete-variable instead.

>  (defvar cfengine3-font-lock-keywords
>    `(
> +    ;; defuns

Please capitalize your comments.

> +    (,(concat "\\<" cfengine3-defuns-regex "\\>"
> +              "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
> +              "[ \t]+\\<\\([[:alnum:]_]+\\)\\((\\([^)]*\\))\\)")
> +    (1 font-lock-builtin-face)
> +    (2 font-lock-constant-face)
> +    (3 font-lock-function-name-face)
> +    (5 font-lock-variable-name-face))
> +
> +    (,(concat "\\<" cfengine3-defuns-regex "\\>"
> +              "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
> +              "[ \t]+\\<\\([[:alnum:]_]+\\)")
> +    (1 font-lock-builtin-face)
> +    (2 font-lock-constant-face)
> +    (3 font-lock-function-name-face))

Why not merge the two entries into something like:

       (,(concat "\\<" cfengine3-defuns-regex "\\>"
                 "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
                 "[ \t]+\\<\\([[:alnum:]_]+\\)\\(?:(\\([^)]*\\))\\)?")
       (1 font-lock-builtin-face)
       (2 font-lock-constant-face)
       (3 font-lock-function-name-face)
       (4 font-lock-variable-name-face nil t))

And please make sure you explain in the commit log (or here in comments)
why this code was moved to the beginning of
cfengine3-font-lock-keywords.


        Stefan