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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 27881 in the body.
You can then email your comments to 27881 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Simen Heggestøyl <simenheg <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Steve Purcell <steve <at> sanityinc.com>, Tom Tromey <tom <at> tromey.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: New major mode: Less mode
Date: Sun, 30 Jul 2017 19:53:03 +0200

[Message part 1 (text/plain, inline)]

Hi.

Steve Purcell has developed a very nice major mode for editing Less
files (a minor variant of CSS). Seeing that Emacs' built-in CSS mode
got extended to support SCSS (another minor CSS variant) three years
ago, I contacted Steve and asked if he'd like Less mode to become a
part of Emacs as well, which he agreed to.

I've attached two patches which merge Steve's Less mode into
css-mode.el. The first patch is a verbatim copy of Steve's code, the
second one contains minor cleanups made by me.

The biggest change I've made is to rename the mode from "Less CSS
mode" to just "Less mode". I think the renaming has two advantages:
Aligning its name with "SCSS mode", and making the mode's commands,
variables and function names shorter. The downside is that existing
users of Less mode will have to update their config files. Maybe
aliasing `less-css-mode' to `less-mode' could help some in that
regard. What do you think of it?

If this looks okay, I'll complete the patches (fix the commit messages
and make a NEWS entry), install them, and start maintaining Less mode
as part of css-mode.el.

-- Simen

[0001-WIP-New-major-mode-Less-mode-a-minor-variant-of-CSS.patch (text/x-patch, attachment)]

[0002-WIP-Fixes-and-tweaks-for-the-new-Less-mode.patch (text/x-patch, attachment)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Simen Heggestøyl <simenheg <at> gmail.com>
Cc: Steve Purcell <steve <at> sanityinc.com>, bug-gnu-emacs <at> gnu.org,
 Tom Tromey <tom <at> tromey.com>, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: New major mode: Less mode
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'.

Message #11 received at 27881 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Simen Heggestøyl <simenheg <at> gmail.com>
Cc: Steve Purcell <steve <at> sanityinc.com>, Tom Tromey <tom <at> tromey.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>, 27881 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#27881: New major mode: Less mode
Date: Sun, 30 Jul 2017 22:02:03 -0400

Simen Heggestøyl wrote:

> The biggest change I've made is to rename the mode from "Less CSS
> mode" to just "Less mode". I think the renaming has two advantages:
> Aligning its name with "SCSS mode", and making the mode's commands,
> variables and function names shorter. The downside is that existing
> users of Less mode will have to update their config files. Maybe
> aliasing `less-css-mode' to `less-mode' could help some in that
> regard. What do you think of it?

FWIW, I would assume that "less-mode" was some sort of file viewer mode
like the standalone less utility.

Message #14 received at 27881 <at> debbugs.gnu.org (full text, mbox):

From: Marcin Borkowski <mbork <at> mbork.pl>
To: Glenn Morris <rgm <at> gnu.org>
Cc: Simen Heggestøyl <simenheg <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>, Steve Purcell <steve <at> sanityinc.com>,
 Tom Tromey <tom <at> tromey.com>, 27881 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#27881: New major mode: Less mode
Date: Mon, 31 Jul 2017 06:32:39 +0200

On 2017-07-31, at 04:02, Glenn Morris <rgm <at> gnu.org> wrote:

> Simen Heggestøyl wrote:
>
>> The biggest change I've made is to rename the mode from "Less CSS
>> mode" to just "Less mode". I think the renaming has two advantages:
>> Aligning its name with "SCSS mode", and making the mode's commands,
>> variables and function names shorter. The downside is that existing
>> users of Less mode will have to update their config files. Maybe
>> aliasing `less-css-mode' to `less-mode' could help some in that
>> regard. What do you think of it?
>
> FWIW, I would assume that "less-mode" was some sort of file viewer mode
> like the standalone less utility.

Exactly my thought, I thought it is "view-mode on steroids"...

-- 
Marcin Borkowski

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Steve Purcell <steve <at> sanityinc.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: bug-gnu-emacs <at> gnu.org, Tom Tromey <tom <at> tromey.com>,
 Simen Heggestøyl <simenheg <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: 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!

Message #20 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Steve Purcell <steve <at> sanityinc.com>
Cc: bug-gnu-emacs <at> gnu.org, Tom Tromey <tom <at> tromey.com>,
 Simen Heggestøyl <simenheg <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: New major mode: Less mode
Date: Mon, 31 Jul 2017 08:44:24 -0400

>>> ;;;###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.

The safety info may need to be autoloaded, indeed (IIUC we have
a misfeature in there: we should enable the major mode (when possible)
before checking safety of the local vars).

But the vars themselves are better not autoloaded.

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

Indeed, if you don't quote it there, it means it's a "shell" string and
can hence only make sense when passed to a shell, rather than used
directly as a file name.

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

Mostly consistency (each style has its quirks).


        Stefan

Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tom Tromey <tom <at> tromey.com>
To: Simen Heggestøyl <simenheg <at> gmail.com>
Cc: Steve Purcell <steve <at> sanityinc.com>, bug-gnu-emacs <at> gnu.org,
 Tom Tromey <tom <at> tromey.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: New major mode: Less mode
Date: Tue, 01 Aug 2017 20:16:59 -0600

>>>>> "Simen" == Simen Heggestøyl <simenheg <at> gmail.com> writes:

Simen> +(defcustom less-css-lessc-command "lessc"
Simen> +  "Command used to compile LESS files.
Simen> +Should be lessc or the complete path to your lessc executable,
Simen> +  e.g.: \"~/.gem/ruby/1.8/bin/lessc\""
Simen> +  :type 'file
Simen> +  :group 'less-css
Simen> +  :safe 'stringp)
Simen> +
Simen> +;;;###autoload
Simen> +(defcustom less-css-compile-at-save nil
Simen> +  "If non-nil, the LESS buffers will be compiled to CSS after each save."
Simen> +  :type 'boolean
Simen> +  :group 'less-css
Simen> +  :safe 'booleanp)

It seems dangerous to me to mark these both as safe.  A file could
specify a dangerous command and then say it's ok to run it on save.

Probably the command should not be :safe.

Maybe the other variables defined after this should also not be safe.

Tom

Message #26 received at 27881 <at> debbugs.gnu.org (full text, mbox):

From: Simen Heggestøyl <simenheg <at> gmail.com>
To: Marcin Borkowski <mbork <at> mbork.pl>
Cc: Glenn Morris <rgm <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Dmitry Gutov <dgutov <at> yandex.ru>, Steve Purcell <steve <at> sanityinc.com>,
 Tom Tromey <tom <at> tromey.com>, 27881 <at> debbugs.gnu.org
Subject: Re: bug#27881: New major mode: Less mode
Date: Thu, 03 Aug 2017 19:47:51 +0200

On Mon, Jul 31, 2017 at 6:32 AM, Marcin Borkowski <mbork <at> mbork.pl> 
wrote:
> 
> On 2017-07-31, at 04:02, Glenn Morris <rgm <at> gnu.org> wrote:
>>  FWIW, I would assume that "less-mode" was some sort of file viewer 
>> mode
>>  like the standalone less utility.
> 
> Exactly my thought, I thought it is "view-mode on steroids"...
> 
> --
> Marcin Borkowski

Hmm, given these comments, I think it might be better to keep Steve's
original name, "Less CSS mode".

-- Simen

Message #29 received at 27881 <at> debbugs.gnu.org (full text, mbox):

From: Simen Heggestøyl <simenheg <at> gmail.com>
To: Tom Tromey <tom <at> tromey.com>
Cc: steve <at> sanityinc.com, 27881 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#27881: New major mode: Less mode
Date: Thu, 03 Aug 2017 19:50:58 +0200

[Message part 1 (text/plain, inline)]

On Sun, Jul 30, 2017 at 10:59 PM, Stefan Monnier 
<monnier <at> IRO.UMontreal.CA> wrote:
> See some comments below.  One thing, tho: an alternative would be to 
put
> it into its own file, which would give it more visibility

Fine by me. Should we perhaps do the same for SCSS mode?


On Mon, Jul 31, 2017 at 8:39 AM, Steve Purcell <steve <at> sanityinc.com> 
wrote:
>  +(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.

I'm not sure either, but the Less compiler didn't like some non-ASCII
characters I fed to it, so I suspect it only allows ASCII characters
in variable names.


On Mon, Jul 31, 2017 at 2:44 PM, Stefan Monnier 
<monnier <at> iro.umontreal.ca> wrote:
> The safety info may need to be autoloaded, indeed (IIUC we have
> a misfeature in there: we should enable the major mode (when 
possible)
> before checking safety of the local vars).

I've removed the autoloads from the defcustoms. How does one autoload
safety info?


On Wed, Aug 2, 2017 at 4:16 AM, Tom Tromey <tom <at> tromey.com> wrote:
> It seems dangerous to me to mark these both as safe.  A file could
> specify a dangerous command and then say it's ok to run it on save.
>
> Probably the command should not be :safe.
>
> Maybe the other variables defined after this should also not be safe.
>
> Tom

I think you're right, that seems particularly unsafe to me too. Maybe
`less-css-output-file-name' shouldn't be marked safe either, since it
may specify the name of an important file can get overwritten without
a confirmation.


I've updated the diffs based on your feedback. Thank you all for your
comments so far.

-- Simen

[0001-WIP-New-major-mode-Less-CSS-mode.patch (text/x-patch, attachment)]

[0002-WIP-Fixes-and-tweaks-for-the-new-Less-CSS-mode.patch (text/x-patch, attachment)]

Message #32 received at 27881 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Simen Heggestøyl <simenheg <at> gmail.com>
Cc: steve <at> sanityinc.com, Tom Tromey <tom <at> tromey.com>, 27881 <at> debbugs.gnu.org,
 dgutov <at> yandex.ru
Subject: Re: bug#27881: New major mode: Less mode
Date: Thu, 03 Aug 2017 16:08:58 -0400

>> See some comments below.  One thing, tho: an alternative would be to
>> put it into its own file, which would give it more visibility
> Fine by me. Should we perhaps do the same for SCSS mode?

We could, but since scss-mode was already included in Emacs-25.1
(i.e. it's been distributed officially for a while now), it's not
as important.

Putting it in its own file makes it possible to distribute it via
GNU ELPA without having to wait for Emacs-26 to be released.

> I've removed the autoloads from the defcustoms. How does one autoload
> safety info?

    ;;;###autoload
    (put '<VAR> 'safe-local-variable '<PRED>)


-- Stefan

Message #35 received at 27881 <at> debbugs.gnu.org (full text, mbox):

From: Richard Stallman <rms <at> gnu.org>
To: Simen Heggestøyl <simenheg <at> gmail.com>
Cc: mbork <at> mbork.pl, monnier <at> iro.umontreal.ca, dgutov <at> yandex.ru,
 steve <at> sanityinc.com, tom <at> tromey.com, 27881 <at> debbugs.gnu.org
Subject: Re: bug#27881: New major mode: Less mode
Date: Fri, 04 Aug 2017 17:33:10 -0400

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Hmm, given these comments, I think it might be better to keep Steve's
  > original name, "Less CSS mode".

I agree.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Message #38 received at 27881 <at> debbugs.gnu.org (full text, mbox):

From: Simen Heggestøyl <simenheg <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: steve <at> sanityinc.com, Tom Tromey <tom <at> tromey.com>, 27881 <at> debbugs.gnu.org,
 dgutov <at> yandex.ru
Subject: Re: bug#27881: New major mode: Less mode
Date: Fri, 11 Aug 2017 00:53:22 +0200

[Message part 1 (text/plain, inline)]

On Thu, Aug 3, 2017 at 10:08 PM, Stefan Monnier 
<monnier <at> IRO.UMontreal.CA> wrote:
>     ;;;###autoload
>     (put '<VAR> 'safe-local-variable '<PRED>)

OK, thanks.

How does it look now?

-- Simen

[0001-New-major-mode-Less-CSS-mode.patch (text/x-patch, attachment)]

[0002-Fixes-and-tweaks-for-the-new-Less-CSS-mode.patch (text/x-patch, attachment)]

Message #41 received at 27881 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Simen Heggestøyl <simenheg <at> gmail.com>
Cc: steve <at> sanityinc.com, Tom Tromey <tom <at> tromey.com>, 27881 <at> debbugs.gnu.org,
 dgutov <at> yandex.ru
Subject: Re: bug#27881: New major mode: Less mode
Date: Mon, 14 Aug 2017 06:06:55 -0400

> How does it look now?

Fine by me,


        Stefan

Message #46 received at 27881-done <at> debbugs.gnu.org (full text, mbox):

From: Simen Heggestøyl <simenheg <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 27881-done <at> debbugs.gnu.org, steve <at> sanityinc.com,
 Tom Tromey <tom <at> tromey.com>, dgutov <at> yandex.ru
Subject: Re: bug#27881: New major mode: Less mode
Date: Tue, 15 Aug 2017 12:33:53 +0200

Installed. Thanks again!

-- Simen

Previous Next

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