GNU bug report logs -
#27881
New major mode: Less mode
Previous Next
Reported by: Simen Heggestøyl <simenheg <at> gmail.com>
Date: Sun, 30 Jul 2017 17:54:01 UTC
Severity: wishlist
Tags: patch
Done: Simen Heggestøyl <simenheg <at> gmail.com>
Bug is archived. No further changes may be made.
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.
Report forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Sun, 30 Jul 2017 17:54:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Simen Heggestøyl <simenheg <at> gmail.com>
:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org
.
(Sun, 30 Jul 2017 17:54:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[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)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Sun, 30 Jul 2017 22:23:02 GMT)
Full text and
rfc822 format available.
Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):
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'.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Mon, 31 Jul 2017 02:03:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 27881 <at> debbugs.gnu.org (full text, mbox):
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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Mon, 31 Jul 2017 04:33:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 27881 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Mon, 31 Jul 2017 06:40:02 GMT)
Full text and
rfc822 format available.
Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):
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!
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Mon, 31 Jul 2017 12:45:02 GMT)
Full text and
rfc822 format available.
Message #20 received at submit <at> debbugs.gnu.org (full text, mbox):
>>> ;;;###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
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Wed, 02 Aug 2017 02:18:02 GMT)
Full text and
rfc822 format available.
Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):
>>>>> "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
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Thu, 03 Aug 2017 17:49:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 27881 <at> debbugs.gnu.org (full text, mbox):
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
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Thu, 03 Aug 2017 17:52:01 GMT)
Full text and
rfc822 format available.
Message #29 received at 27881 <at> debbugs.gnu.org (full text, mbox):
[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)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Thu, 03 Aug 2017 20:10:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 27881 <at> debbugs.gnu.org (full text, mbox):
>> 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
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Fri, 04 Aug 2017 21:35:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 27881 <at> debbugs.gnu.org (full text, mbox):
[[[ 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.
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Thu, 10 Aug 2017 22:54:02 GMT)
Full text and
rfc822 format available.
Message #38 received at 27881 <at> debbugs.gnu.org (full text, mbox):
[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)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org
:
bug#27881
; Package
emacs
.
(Mon, 14 Aug 2017 10:07:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 27881 <at> debbugs.gnu.org (full text, mbox):
> How does it look now?
Fine by me,
Stefan
Reply sent
to
Simen Heggestøyl <simenheg <at> gmail.com>
:
You have taken responsibility.
(Tue, 15 Aug 2017 10:35:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Simen Heggestøyl <simenheg <at> gmail.com>
:
bug acknowledged by developer.
(Tue, 15 Aug 2017 10:35:02 GMT)
Full text and
rfc822 format available.
Message #46 received at 27881-done <at> debbugs.gnu.org (full text, mbox):
Installed. Thanks again!
-- Simen
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Tue, 12 Sep 2017 11:24:03 GMT)
Full text and
rfc822 format available.
This bug report was last modified 7 years and 284 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.