From: Stefan Monnier <monnier@IRO.UMontreal.CA>
To: emacs-devel@gnu.org
Subject: Re: changes to cfengine-mode
Date: Wed, 07 Dec 2011 11:10:23 -0500 [thread overview]
Message-ID: <jwvborkbdab.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <8739cwtzh8.fsf@lifelogs.com> (Ted Zlatanov's message of "Wed, 07 Dec 2011 06:04:03 -0500")
>>> 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
next prev parent reply other threads:[~2011-12-07 16:10 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-11-22 17:18 prog-mode not available in earlier Emacsen, need help with cfengine.el Ted Zlatanov
2011-11-22 18:05 ` Eli Zaretskii
2011-11-22 18:45 ` Ted Zlatanov
2011-11-22 22:30 ` Stefan Monnier
2011-11-23 2:09 ` Ted Zlatanov
2011-11-23 6:03 ` Chong Yidong
2011-11-23 13:35 ` Ted Zlatanov
2011-11-24 6:03 ` Chong Yidong
2011-11-25 13:17 ` Ted Zlatanov
2011-11-26 10:57 ` Reiner Steib
2011-11-27 2:06 ` Ted Zlatanov
2011-12-01 15:39 ` Ted Zlatanov
2011-12-02 16:12 ` changes to cfengine-mode (was: prog-mode not available in earlier Emacsen, need help with cfengine.el) Ted Zlatanov
2011-12-02 20:32 ` changes to cfengine-mode Stefan Monnier
2011-12-07 11:04 ` Ted Zlatanov
2011-12-07 16:10 ` Stefan Monnier [this message]
2011-12-08 9:52 ` Chong Yidong
2011-12-13 17:21 ` Ted Zlatanov
2011-12-13 19:51 ` Stefan Monnier
2011-12-13 21:46 ` Ted Zlatanov
2011-12-17 0:45 ` Stefan Monnier
2011-12-21 11:31 ` Ted Zlatanov
2011-12-13 17:23 ` Ted Zlatanov
2011-12-07 16:12 ` Stefan Monnier
2011-12-13 15:47 ` Ted Zlatanov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=jwvborkbdab.fsf-monnier+emacs@gnu.org \
--to=monnier@iro.umontreal.ca \
--cc=emacs-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.