Subject: bug#27881: New major mode: Less mode

From: Stefan Monnier <[email-masked]>
Date: Sun, 30 Jul 2017 16:59:57 -0400

Thanks.  Looks good.

See some comments below.  One thing, tho: an alternative would be to put
it into its own file, which would give it more visibility

        Stefan

> ;; This mode provides syntax highlighting for LESS CSS files, plus

I'd add a URL pointing to the "canonical" place for Less.

> -(defgroup less-css nil
> -  "Less-css mode"
> -  :prefix "less-css-"
> +(defgroup less nil
> +  "Less CSS mode"
> +  :prefix "less-"
>    :group 'css)
 
>  ;;;###autoload
> -(defcustom less-css-lessc-command "lessc"

Why autoload all these defcustom.  Sounds like a bug.

>    "File name in which to save CSS, or nil to use <name>.css for <name>.less.
> -
>  This can be also be set to a full path, or a relative path.  If
>  the path is relative, it will be relative to the value of
> -`less-css-output-dir', if set, or the current directory by
> -default."
> +`less-output-dir', if set, or the current directory by default."
>    :type 'file
> -  :group 'less-css
> +  :group 'less
>    :safe 'stringp)

Since these defcusotm come right after the (defgroup less ..), the

    :group 'less

is redundant.

>  (add-to-list 'compilation-error-regexp-alist-alist
> +             (list 'less less-default-error-regex 2 3 4 nil 1))

BTW, I recommend reporting the use of an ad-hoc error format as an error
to the Less developers.

>  ;;;###autoload
> +(defun less-compile ()

Why autoload?  This operates on the current buffer, so presumably this
buffer is already in less-mode.

> +  (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?

> +            (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).

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

> +(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'.