GNU bug report logs - #27881
New major mode: Less mode

Previous Next

Full log

View this message in rfc822 format

From: Steve Purcell <steve <at> sanityinc.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 27881 <at> debbugs.gnu.org, tom <at> tromey.com, simenheg <at> gmail.com, dgutov <at> yandex.ru
Subject: bug#27881: New major mode: Less mode
Date: Mon, 31 Jul 2017 18:39:23 +1200

Comments below:

>> ;;;###autoload
>> -(defcustom less-css-lessc-command "lessc"
> 
> Why autoload all these defcustom.  Sounds like a bug.


It may well be superfluous, but I found a note that this was added because non-autoloaded custom vars wouldn’t be considered safe as dir local variables otherwise.


> Since these defcusotm come right after the (defgroup less ..), the
> 
>    :group 'less
> 
> is redundant.


Agree.


>> ;;;###autoload
>> +(defun less-compile ()
> 
> Why autoload?  This operates on the current buffer, so presumably this
> buffer is already in less-mode.

Probably, yes.


>> +  (message "Compiling Less to CSS")
> 
> I notice that "Less" is used pretty much everywhere except in the
> define-derived-mode where Steve used "LESS".  Why not use "Less" in the
> mode name as well?


Agree, consistency would be good. Official upstream name is “Less”.


>> +            (append (list (less--maybe-shell-quote-command less-lessc-command))
> 
> I'd recommend to just never quote less-lessc-command rather than only
> quote it on non-Windows systems.  It's actually more flexible this way
> since less-lessc-command can then be set to something fancier (e.g. the
> command with a few flags).


This fixed a concrete bug, but yes, it’s probably misguided: it would have been better to always leave it unquoted.

However, I wouldn’t advocate adding arbitrary flags to a command like this, because it can’t then be passed to one of the “process” functions that expects the first of its args to be the path of the executable, right?


>> +(defconst less-font-lock-keywords
>>   '(;; Variables
>> -    ("@[a-z_-][a-z-_0-9]*" . font-lock-constant-face)
>> +    ("@[a-z_-][a-z-_0-9]*" . font-lock-variable-name-face)
>>     ("&" . font-lock-preprocessor-face)
>>     ;; Mixins
>> -    ("\\(?:[ \t{;]\\|^\\)\\(\\.[a-z_-][a-z-_0-9]*\\)[ \t]*;" . (1 font-lock-keyword-face)))
>> -  )
>> +    ("\\(?:[ \t{;]\\|^\\)\\(\\.[a-z_-][a-z-_0-9]*\\)[ \t]*;" .
>> +     (1 font-lock-keyword-face))))
> 
> Is it important to limit those to ASCII chars?  If not, then it's better
> to use [[:alpha:]_-] and [[:alnum:]_-] in the above regexps.


Unsure, but it’s likely safe to allow non-ASCII alphanumeric chars. 


>> +(define-key less-mode-map "\C-c\C-c" 'less-compile)
> 
> The standard way is to do:
> 
>    (defvar less-mode-map
>      (let ((map (make-sparse-keymap))))
>        (define-key map "\C-c\C-c" 'less-compile)
>        map)
> 
> before the `define-derived-mode’.


Is there an advantage to this? My reasoning is that “define-derived-mode” generates the mode map declaration, so why not fill it afterwards. If consistency is important, though, this would be a reasonable thing to change.

Thanks for the review!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.