Re: changes to cfengine-mode

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

>>> I'd like to rename `cfengine-mode' to `cfengine2-mode', and
>>> `cfengine-auto-mode' to `cfengine-mode' (`cfengine-auto-mode' loads
>>> `cfengine3-mode' if it thinks a cfengine v.3 file is edited).
SM> That sounds right.
> Thanks for looking.  Attached is a patch that will:
> 1) rename `cfengine-mode' to `cfengine2-mode' and defalias
> 'cfengine-mode to 'cfengine-auto-mode.  Provide 'cfengine2.

That's good.

> 2) rename all the "cfengine-*" variables used only by `cfengine2-mode'
> to "cfengine2-*"

Is this needed?  Do these variables only make sense for cfengine2 syntax?

> 3) adjust the commentary to match

Good.

> 4) give a version and set it to 1.1 for this release

We don't like versions for files bundled with Emacs (only serves to
cause spurious merge conflicts), so if you can keep this outside of the
tree, it's preferable.

> 5) use "CFEngine" consistently, capitalized as the author, Mark Burgess,
> does.

Good.

> 6) add `cfengine3-mode-verbose' to control some debugging info

Why not `cfengine-mode-verbose'?  Also, if it's for debugging, why
call it "verbose" rather than "debug"?

> 7) add `cfengine3-parameters-indent' to address some shortcomings in
> indentation.  This is a new feature but necessary to fix the lack of a
> hanging indent in the current `cfengine3-mode'.  It's very low-risk and
> I hope it's acceptable.

I'd keep it with a "cfengine-" prefix.  Basically, I'd keep
user-facing names with a "cfengine-" prefix whenever possible.

> 8) improve and capitalize comments in many places, though that work is
> not complete

Good.

> 9) change the "lighter" modeline indicators from "Cfengine[23]" to
> "CFE[23]" to take less space

No opinion on this.

> I also have a font-lock issue I can't figure out with the current
> version.  Given the following code (use `cfengine3-mode' to see the
> problem):

*Messages* tells me:
Error during redisplay: (void-variable font-lock-constant-name-face)

Some comments on the code follow.


        Stefan


> +(defvar cfengine-version nil)
> +(setq cfengine-version "1.1")

Regardless of whether we keep this, the above is atrocious.  Use `defconst'.

> +(defcustom cfengine3-parameters-indent '(promise pname 0)
> +  "*Indentation of CFEngine3 promise parameters (hanging indent)."

This description is incomprehensible to me.  It should describe the
accepted values and their meaning.  Also better include a short example
to make it clear what this controls.

> +(defcustom cfengine2-mode-abbrevs nil
> +  "Abbrevs for CFEngine2 mode."
>    :group 'cfengine
>    :type '(repeat (list (string :tag "Name")
>  		       (string :tag "Expansion")

You haven't added the corresponding feature for cfengine3, and I think
I agree with this choice, but the reason is not that it made sense for
cfengine2 and it doesn't for cfengine3, but simply that there's no need
for a config var for it.  There's already `edit-abbrevs'.
So rather than rename it, I'd obsolete it and mention that it only
applies to cfengine2-mode.

> -  (defconst cfengine-actions
> +  (defconst cfengine2-actions

I think cfengine-font-lock-keywords also needs to be renamed to
cfengine2 prefix.

>  (defvar cfengine3-font-lock-keywords
>    `(
> -    (,(concat "^[ \t]*" cfengine3-class-selector-regex)
> -     1 font-lock-keyword-face)
> -    (,(concat "^[ \t]*" cfengine3-category-regex)
> -     1 font-lock-builtin-face)
> -    ;; Variables, including scope, e.g. module.var
> -    ("[@$](\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
> -    ("[@$]{\\([[:alnum:]_.]+\\)}" 1 font-lock-variable-name-face)
> -    ;; Variable definitions.
> -    ("\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1 font-lock-variable-name-face)
> -
> -    ;; CFEngine 3.x faces
>      ;; defuns
>      (,(concat "\\<" cfengine3-defuns-regex "\\>"
>                "[ \t]+\\<\\([[:alnum:]_]+\\)\\>"
> @@ -136,27 +144,43 @@
>      (2 font-lock-constant-name-face)
>      (3 font-lock-function-name-face)
>      (5 font-lock-variable-name-face))
> +
> +    ;; class selector
> +    (,(concat "^[ \t]*" cfengine3-class-selector-regex)
> +     1 font-lock-keyword-face)
> +
> +    ;; category
> +    (,(concat "^[ \t]*" cfengine3-category-regex)
> +     1 font-lock-builtin-face)
> +
> +    ;; Variables, including scope, e.g. module.var
> +    ("[@$](\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
> +    ("[@$]{\\([[:alnum:]_.]+\\)}" 1 font-lock-variable-name-face)
> +
> +    ;; Variable definitions.
> +    ("\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1 font-lock-variable-name-face)
> +
>      ;; variable types
>      (,(concat "\\<" (eval-when-compile (regexp-opt cfengine3-vartypes t)) "\\>")
>       1 font-lock-type-face)))

Not sure what this is about.  You'd need to describe it in
the ChangeLog.  The comments should be capitalized.

> +(provide 'cfengine2)

Don't bother.


        Stefan