[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#27881: New major mode: Less mode
From: |
Stefan Monnier |
Subject: |
bug#27881: New major mode: Less mode |
Date: |
Sun, 30 Jul 2017 16:59:57 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) |
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'.