From: Steve Purcell <steve@sanityinc.com>
To: Stefan Monnier <monnier@IRO.UMontreal.CA>
Cc: tom@tromey.com, dgutov@yandex.ru, 27881@debbugs.gnu.org,
simenheg@gmail.com
Subject: bug#27881: New major mode: Less mode
Date: Mon, 31 Jul 2017 18:39:23 +1200 [thread overview]
Message-ID: <E692049D-47DF-438C-8399-92BF97F637F5@sanityinc.com> (raw)
In-Reply-To: <jwvy3r5ejwk.fsf-monnier+emacsbugs@gnu.org>
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!
next prev parent reply other threads:[~2017-07-31 6:39 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-30 17:53 bug#27881: New major mode: Less mode Simen Heggestøyl
2017-07-30 20:59 ` Stefan Monnier
2017-07-31 6:39 ` Steve Purcell [this message]
2017-07-31 12:44 ` Stefan Monnier
2017-07-31 2:02 ` Glenn Morris
2017-07-31 4:32 ` Marcin Borkowski
2017-08-03 17:47 ` Simen Heggestøyl
2017-08-04 21:33 ` Richard Stallman
2017-08-02 2:16 ` Tom Tromey
2017-08-03 17:50 ` Simen Heggestøyl
2017-08-03 20:08 ` Stefan Monnier
2017-08-10 22:53 ` Simen Heggestøyl
2017-08-14 10:06 ` Stefan Monnier
2017-08-15 10:33 ` Simen Heggestøyl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E692049D-47DF-438C-8399-92BF97F637F5@sanityinc.com \
--to=steve@sanityinc.com \
--cc=27881@debbugs.gnu.org \
--cc=dgutov@yandex.ru \
--cc=monnier@IRO.UMontreal.CA \
--cc=simenheg@gmail.com \
--cc=tom@tromey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.