[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#27881: New major mode: Less mode
From: |
Steve Purcell |
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!