Re: changes to cfengine-mode

[Ted Zlatanov]

On Wed, 07 Dec 2011 11:10:23 -0500 Stefan Monnier <address@hidden> wrote: 

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

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

Yes, I renamed those that are only for cfengine2.

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

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

I'm OK either way. The attached patch still has it as per Chong's
comment.

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

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

Fixed.

>> 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.

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

Fixed (but it will stay out until 24.2).

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

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

Removed.

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

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

Fixed.

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

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

OK; done.

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

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

OK.

>> (defvar cfengine3-font-lock-keywords
...
SM> Not sure what this is about.  You'd need to describe it in
SM> the ChangeLog.  The comments should be capitalized.

I'm not sure what you mean?  It's the font-lock keywords?

>> +(provide 'cfengine2)

SM> Don't bother.

OK.

I'll commit the revised code after 24.1 is out.  There's no need for it
sooner, unless you want it.  In that case I'll remove
`cfengine-parameter-indent'.

Ted

=== modified file 'lisp/progmodes/cfengine.el'
--- lisp/progmodes/cfengine.el  2011-11-20 03:48:53 +0000
+++ lisp/progmodes/cfengine.el  2011-12-13 17:09:20 +0000
@@ -5,6 +5,7 @@
 ;; Author: Dave Love <address@hidden>
 ;; Maintainer: Ted Zlatanov <address@hidden>
 ;; Keywords: languages
+;; Version: 1.1
 
 ;; This file is part of GNU Emacs.
 
@@ -29,18 +30,18 @@
 ;; The CFEngine 3.x support doesn't have Imenu support but patches are
 ;; welcome.
 
-;; You can set it up so either cfengine-mode (2.x and earlier) or
-;; cfengine3-mode (3.x) will be picked, depending on the buffer
+;; You can set it up so either `cfengine2-mode' (2.x and earlier) or
+;; `cfengine3-mode' (3.x) will be picked, depending on the buffer
 ;; contents:
 
-;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine-auto-mode))
+;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine-mode))
 
 ;; OR you can choose to always use a specific version, if you prefer
-;; it
+;; it:
 
 ;; (add-to-list 'auto-mode-alist '("\\.cf\\'" . cfengine3-mode))
-;; (add-to-list 'auto-mode-alist '("^cf\\." . cfengine-mode))
-;; (add-to-list 'auto-mode-alist '("^cfagent.conf\\'" . cfengine-mode))
+;; (add-to-list 'auto-mode-alist '("^cf\\." . cfengine2-mode))
+;; (add-to-list 'auto-mode-alist '("^cfagent.conf\\'" . cfengine2-mode))
 
 ;; This is not the same as the mode written by Rolf Ebert
 ;; <address@hidden>, distributed with cfengine-2.0.5.  It does
@@ -49,24 +50,96 @@
 ;;; Code:
 
 (defgroup cfengine ()
-  "Editing Cfengine files."
+  "Editing CFEngine files."
   :group 'languages)
 
 (defcustom cfengine-indent 2
-  "*Size of a Cfengine indentation step in columns."
+  "*Size of a CFEngine indentation step in columns."
   :group 'cfengine
   :type 'integer)
 
+(defcustom cfengine-parameters-indent '(promise pname 0)
+  "*Indentation of CFEngine3 promise parameters (hanging indent).
+
+For example, say you have this code:
+
+bundle x y
+{
+  section:
+    class::
+      promise ...
+      promiseparameter => ...
+}
+
+You can choose to indent promiseparameter from the beginning of
+the line (absolutely) or from the word \"promise\" (relatively).
+
+You can also choose to indent the start of the word
+\"promiseparameter\" or the arrow that follows it.
+
+Finally, you can choose the amount of the indent.
+
+The default is to anchor at promise, indent parameter name, and offset 0:
+
+bundle agent rcfiles
+{
+  files:
+    any::
+      \"/tmp/netrc\"
+      comment => \"my netrc\",
+      perms => mog(\"600\", \"tzz\", \"tzz\");
+}
+
+Here we anchor at beginning of line, indent arrow, and offset 10:
+
+bundle agent rcfiles
+{
+  files:
+    any::
+      \"/tmp/netrc\"
+  comment => \"my netrc\",
+    perms => mog(\"600\", \"tzz\", \"tzz\");
+}
+
+Some, including cfengine_stdlib.cf, like to anchor at promise, indent
+arrow, and offset 16 or so:
+
+bundle agent rcfiles
+{
+  files:
+    any::
+      \"/tmp/netrc\"
+              comment => \"my netrc\",
+                perms => mog(\"600\", \"tzz\", \"tzz\");
+}
+"
+  :group 'cfengine
+  :type '(list
+          (choice (const :tag "Anchor at beginning of promise" promise)
+                  (const :tag "Anchor at beginning of line" bol))
+          (choice (const :tag "Indent parameter name" pname)
+                  (const :tag "Indent arrow" arrow))
+          (integer :tag "Indentation amount from anchor")))
+
+(defcustom cfengine-mode-debug nil
+  "Whether `cfengine-mode' should print debugging info
+ (only used by the cfengine3 support currently)"
+  :group 'cfengine
+  :type 'boolean)
+
 (defcustom cfengine-mode-abbrevs nil
-  "Abbrevs for Cfengine mode."
+  "Abbrevs for CFEngine2 mode.
+Obsolete, see `edit-abbrevs' instead."
   :group 'cfengine
   :type '(repeat (list (string :tag "Name")
                       (string :tag "Expansion")
                       (choice  :tag "Hook" (const nil) function))))
 
+(make-obsolete-variable 'cfengine-mode-abbrevs nil "24.1")
+
 ;; Taken from the doc for pre-release 2.1.
 (eval-and-compile
-  (defconst cfengine-actions
+  (defconst cfengine2-actions
     '("acl" "alerts" "binservers" "broadcast" "control" "classes" "copy"
       "defaultroute" "disks" "directories" "disable" "editfiles" "files"
       "filters" "groups" "homeservers" "ignore" "import" "interfaces"
@@ -98,11 +171,11 @@
      '(string int real slist ilist rlist irange rrange counter))
     "List of the CFEngine 3.x variable types."))
 
-(defvar cfengine-font-lock-keywords
+(defvar cfengine2-font-lock-keywords
   `(;; Actions.
     ;; List the allowed actions explicitly, so that errors are more obvious.
     (,(concat "^[ \t]*" (eval-when-compile
-                         (regexp-opt cfengine-actions t))
+                         (regexp-opt cfengine2-actions t))
              ":")
      1 font-lock-keyword-face)
     ;; Classes.
@@ -117,17 +190,6 @@
 
 (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
-    ("address@hidden(\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
-    ("address@hidden([[: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 +198,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
+    ("address@hidden(\\([[:alnum:]_.]+\\))" 1 font-lock-variable-name-face)
+    ("address@hidden([[: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)))
 
-(defvar cfengine-imenu-expression
+(defvar cfengine2-imenu-expression
   `((nil ,(concat "^[ \t]*" (eval-when-compile
-                             (regexp-opt cfengine-actions t))
+                             (regexp-opt cfengine2-actions t))
                  ":[^:]")
         1)
     ("Variables/classes" "\\<\\([[:alnum:]_]+\\)[ \t]*=[ \t]*(" 1)
     ("Variables/classes" "\\<define=\\([[:alnum:]_]+\\)" 1)
     ("Variables/classes" "\\<DefineClass\\>[ \t]+\\([[:alnum:]_]+\\)" 1))
-  "`imenu-generic-expression' for Cfengine mode.")
+  "`imenu-generic-expression' for CFEngine mode.")
 
-(defun cfengine-outline-level ()
-  "`outline-level' function for Cfengine mode."
+(defun cfengine2-outline-level ()
+  "`outline-level' function for CFEngine mode."
   (if (looking-at "[^:]+\\(?:[:]+\\)$")
       (length (match-string 1))))
 
-(defun cfengine-beginning-of-defun ()
-  "`beginning-of-defun' function for Cfengine mode.
+(defun cfengine2-beginning-of-defun ()
+  "`beginning-of-defun' function for CFEngine mode.
 Treats actions as defuns."
   (unless (<= (current-column) (current-indentation))
     (end-of-line))
@@ -165,8 +243,8 @@
     (goto-char (point-min)))
   t)
 
-(defun cfengine-end-of-defun ()
-  "`end-of-defun' function for Cfengine mode.
+(defun cfengine2-end-of-defun ()
+  "`end-of-defun' function for CFEngine mode.
 Treats actions as defuns."
   (end-of-line)
   (if (re-search-forward "^[[:alpha:]]+: *$" nil t)
@@ -176,7 +254,7 @@
 
 ;; Fixme: Should get an extra indent step in editfiles BeginGroup...s.
 
-(defun cfengine-indent-line ()
+(defun cfengine2-indent-line ()
   "Indent a line in Cfengine mode.
 Intended as the value of `indent-line-function'."
   (let ((pos (- (point-max) (point))))
@@ -283,15 +361,17 @@
       (narrow-to-defun)
       (back-to-indentation)
       (setq parse (parse-partial-sexp (point-min) (point)))
-      (message "%S" parse)
+      (when cfengine-mode-debug
+        (message "%S" parse))
+
       (cond
-       ;; body/bundle blocks start at 0
+       ;; Body/bundle blocks start at 0.
        ((looking-at (concat cfengine3-defuns-regex "\\>"))
         (indent-line-to 0))
-       ;; categories are indented one step
+       ;; Categories are indented one step.
        ((looking-at (concat cfengine3-category-regex "[ \t]*$"))
         (indent-line-to cfengine-indent))
-       ;; class selectors are indented two steps
+       ;; Class selectors are indented two steps.
        ((looking-at (concat cfengine3-class-selector-regex "[ \t]*$"))
         (indent-line-to (* 2 cfengine-indent)))
        ;; Outdent leading close brackets one step.
@@ -303,13 +383,31 @@
                               (backward-sexp)
                               (current-column)))
           (error nil)))
-       ;; inside a string and it starts before this line
+       ;; Inside a string and it starts before this line.
        ((and (nth 3 parse)
              (< (nth 8 parse) (save-excursion (beginning-of-line) (point))))
         (indent-line-to 0))
-       ;; inside a defun, but not a nested list (depth is 1)
+
+       ;; Inside a defun, but not a nested list (depth is 1).  This is
+       ;; a promise, usually.
        ((= 1 (nth 0 parse))
-        (indent-line-to (* (+ 2 (nth 0 parse)) cfengine-indent)))
+        (let ((p-anchor (nth 0 cfengine-parameters-indent))
+              (p-what (nth 1 cfengine-parameters-indent))
+              (p-indent (nth 2 cfengine-parameters-indent)))
+          ;; Do we have the parameter anchor and location and indent
+          ;; defined, and are we looking at a promise parameter?
+          (if (and p-anchor p-what p-indent
+                   (looking-at  "\\([[:alnum:]_]+[ \t]*\\)=>"))
+              (let* ((arrow-offset (* -1 (length (match-string 1))))
+                     (extra-offset (if (eq p-what 'arrow) arrow-offset 0))
+                     (base-offset (if (eq p-anchor 'promise)
+                                      (* (+ 2 (nth 0 parse)) cfengine-indent)
+                                    0)))
+                (indent-line-to (max 0 (+ p-indent base-offset extra-offset))))
+            ;; Else, indent to cfengine-indent times the nested depth
+            ;; plus 2.  That way, promises indent deeper than class
+            ;; selectors, which in turn are one deeper than categories.
+          (indent-line-to (* (+ 2 (nth 0 parse)) cfengine-indent)))))
        ;; Inside brackets/parens: indent to start column of non-comment
        ;; token on line following open bracket or by one step from open
        ;; bracket's column.
@@ -421,8 +519,8 @@
   (modify-syntax-entry ?\\ "." table))
 
 ;;;###autoload
-(define-derived-mode cfengine3-mode prog-mode "CFEngine3"
-  "Major mode for editing cfengine input.
+(define-derived-mode cfengine3-mode prog-mode "CFE3"
+  "Major mode for editing CFEngine3 input.
 There are no special keybindings by default.
 
 Action blocks are treated as defuns, i.e. \\[beginning-of-defun] moves
@@ -441,39 +539,39 @@
        #'cfengine3-end-of-defun))
 
 ;;;###autoload
-(define-derived-mode cfengine-mode prog-mode "Cfengine"
-  "Major mode for editing cfengine input.
+(define-derived-mode cfengine2-mode prog-mode "CFE2"
+  "Major mode for editing CFEngine2 input.
 There are no special keybindings by default.
 
 Action blocks are treated as defuns, i.e. \\[beginning-of-defun] moves
 to the action header."
   (cfengine-common-settings)
-  (cfengine-common-syntax cfengine-mode-syntax-table)
+  (cfengine-common-syntax cfengine2-mode-syntax-table)
 
   ;; Shell commands can be quoted by single, double or back quotes.
   ;; It's debatable whether we should define string syntax, but it
   ;; should avoid potential confusion in some cases.
-  (modify-syntax-entry ?\' "\"" cfengine-mode-syntax-table)
-  (modify-syntax-entry ?\` "\"" cfengine-mode-syntax-table)
+  (modify-syntax-entry ?\' "\"" cfengine2-mode-syntax-table)
+  (modify-syntax-entry ?\` "\"" cfengine2-mode-syntax-table)
 
-  (set (make-local-variable 'indent-line-function) #'cfengine-indent-line)
+  (set (make-local-variable 'indent-line-function) #'cfengine2-indent-line)
   (set (make-local-variable 'outline-regexp) "[ \t]*\\(\\sw\\|\\s_\\)+:+")
-  (set (make-local-variable 'outline-level) #'cfengine-outline-level)
+  (set (make-local-variable 'outline-level) #'cfengine2-outline-level)
   (set (make-local-variable 'fill-paragraph-function)
        #'cfengine-fill-paragraph)
-  (define-abbrev-table 'cfengine-mode-abbrev-table cfengine-mode-abbrevs)
+  (define-abbrev-table 'cfengine2-mode-abbrev-table cfengine-mode-abbrevs)
   (setq font-lock-defaults
-       '(cfengine-font-lock-keywords nil nil nil beginning-of-line))
+        '(cfengine2-font-lock-keywords nil nil nil beginning-of-line))
   ;; Fixme: set the args of functions in evaluated classes to string
   ;; syntax, and then obey syntax properties.
-  (setq imenu-generic-expression cfengine-imenu-expression)
+  (setq imenu-generic-expression cfengine2-imenu-expression)
   (set (make-local-variable 'beginning-of-defun-function)
-       #'cfengine-beginning-of-defun)
-  (set (make-local-variable 'end-of-defun-function) #'cfengine-end-of-defun))
+       #'cfengine2-beginning-of-defun)
+  (set (make-local-variable 'end-of-defun-function) #'cfengine2-end-of-defun))
 
 ;;;###autoload
 (defun cfengine-auto-mode ()
-  "Choose between `cfengine-mode' and `cfengine3-mode' depending
+  "Choose between `cfengine2-mode' and `cfengine3-mode' depending
 on the buffer contents"
   (let ((v3 nil))
     (save-restriction
@@ -481,7 +579,9 @@
       (while (not (or (eobp) v3))
         (setq v3 (looking-at (concat cfengine3-defuns-regex "\\>")))
         (forward-line)))
-    (if v3 (cfengine3-mode) (cfengine-mode))))
+    (if v3 (cfengine3-mode) (cfengine2-mode))))
+
+(defalias 'cfengine-mode 'cfengine-auto-mode)
 
 (provide 'cfengine3)
 (provide 'cfengine)