On Wed, 07 Dec 2011 11:10:23 -0500 Stefan Monnier <monnier@IRO.UMontreal.CA> 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