[PATCH] emacs: devel: Highlight 'modify-phases' keywords.

[PATCH] emacs: devel: Highlight 'modify-phases' keywords.

[Alex Kost]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

Hello, I would like to add a usual keyword highlighting for some Guix
things (substitute-keyword-arguments, with-directory-excursion, etc.).

But this one is unusual, as along with 'modify-phases' itself, its
keywords ('add-after', 'delete', …) will also be highlighted.  IMO this
will make the code of the packages easier to read; or is it too
colorful?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-emacs-devel-Highlight-modify-phases-keywords.patch --]
[-- Type: text/x-diff, Size: 4121 bytes --]

From ed1c3871b81d4904d106a3ca5aadde2e1803d2a6 Mon Sep 17 00:00:00 2001
From: Alex Kost <alezost@gmail.com>
Date: Thu, 24 Sep 2015 20:10:29 +0300
Subject: [PATCH] emacs: devel: Highlight 'modify-phases' keywords.

* emacs/guix-guile.el (guix-guile-keyword-regexp): New function.
* emacs/guix-devel.el (guix-devel-faces): New custom group.
  (guix-devel-modify-phases-keyword): New face.
  (guix-devel-modify-phases-keyword-regexp,
  guix-devel-font-lock-keywords): New variables.
  (guix-devel-modify-phases-font-lock-matcher,
  guix-devel-modify-phases-font-lock-pre): New functions.
  (guix-devel-mode): Adjust to add/remove font-lock-keywords.
---
 emacs/guix-devel.el | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 emacs/guix-guile.el |  4 ++++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/emacs/guix-devel.el b/emacs/guix-devel.el
index 6de49be..8a6fc1e 100644
--- a/emacs/guix-devel.el
+++ b/emacs/guix-devel.el
@@ -33,6 +33,16 @@
   "Settings for Guix development utils."
   :group 'guix)
 
+(defgroup guix-devel-faces nil
+  "Faces for `guix-devel-mode'."
+  :group 'guix-devel
+  :group 'guix-faces)
+
+(defface guix-devel-modify-phases-keyword
+  '((t :inherit font-lock-preprocessor-face))
+  "Face for a `modify-phases' keyword ('delete', 'replace', etc.)."
+  :group 'guix-devel-faces)
+
 (defcustom guix-devel-activate-mode t
   "If non-nil, then `guix-devel-mode' is automatically activated
 in Scheme buffers."
@@ -94,6 +104,41 @@ Interactively, use the module defined by the current scheme file."
                                       guix-use-substitutes)
                 "#:dry-run?" (guix-guile-boolean guix-dry-run)))))))
 
+\f
+;;; Font-lock
+
+(defvar guix-devel-modify-phases-keyword-regexp
+  (rx (+ word))
+  "Regexp for a 'modify-phases' keyword ('delete', 'replace', etc.).")
+
+(defun guix-devel-modify-phases-font-lock-matcher (limit)
+  "Find a 'modify-phases' keyword.
+This function is used as a MATCHER for `font-lock-keywords'."
+  (ignore-errors
+    (down-list)
+    (or (re-search-forward guix-devel-modify-phases-keyword-regexp
+                           limit t)
+        (set-match-data nil))
+    (up-list)
+    t))
+
+(defun guix-devel-modify-phases-font-lock-pre ()
+  "Skip the next sexp, and return the end point of the current list.
+This function is used as a PRE-MATCH-FORM for `font-lock-keywords'
+to find 'modify-phases' keywords."
+  (ignore-errors (forward-sexp))
+  (save-excursion (up-list) (point)))
+
+(defvar guix-devel-font-lock-keywords
+  `((,(guix-guile-keyword-regexp "modify-phases")
+     (1 'font-lock-keyword-face)
+     (guix-devel-modify-phases-font-lock-matcher
+      (guix-devel-modify-phases-font-lock-pre)
+      nil
+      (0 'guix-devel-modify-phases-keyword nil t))))
+  "A list of `font-lock-keywords' for `guix-devel-mode'.")
+
+\f
 (defvar guix-devel-keys-map
   (let ((map (make-sparse-keymap)))
     (define-key map (kbd "b") 'guix-devel-build-package-definition)
@@ -122,7 +167,15 @@ bindings:
 \\{guix-devel-mode-map}"
   :init-value nil
   :lighter " Guix"
-  :keymap guix-devel-mode-map)
+  :keymap guix-devel-mode-map
+  (if guix-devel-mode
+      (progn
+        (setq-local font-lock-multiline t)
+        (font-lock-add-keywords nil guix-devel-font-lock-keywords))
+    (setq-local font-lock-multiline nil)
+    (font-lock-remove-keywords nil guix-devel-font-lock-keywords))
+  (when font-lock-mode
+    (font-lock-fontify-buffer)))
 
 ;;;###autoload
 (defun guix-devel-activate-mode-maybe ()
diff --git a/emacs/guix-guile.el b/emacs/guix-guile.el
index 35a97d7..63322d7 100644
--- a/emacs/guix-guile.el
+++ b/emacs/guix-guile.el
@@ -59,6 +59,10 @@ Return nil, if current buffer does not define a module."
 Transform elisp ARG (nil or non-nil) to the guile boolean (#f or #t)."
   (concat "#" (prin1-to-string (if arg 't 'f))))
 
+(defun guix-guile-keyword-regexp (keyword)
+  "Return regexp to find guile KEYWORD."
+  (format "(\\(%s\\)\\_>" keyword))
+
 (defun guix-guile-make-call-expression (proc &rest args)
   "Return \"(PROC ARGS ...)\" string.
 PROC and ARGS should be strings."
-- 
2.5.1

Re: [PATCH] emacs: devel: Highlight 'modify-phases' keywords.

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> Hello, I would like to add a usual keyword highlighting for some Guix
> things (substitute-keyword-arguments, with-directory-excursion, etc.).
>
> But this one is unusual, as along with 'modify-phases' itself, its
> keywords ('add-after', 'delete', …) will also be highlighted.  IMO this
> will make the code of the packages easier to read; or is it too
> colorful?

Sounds like a good idea!

> From ed1c3871b81d4904d106a3ca5aadde2e1803d2a6 Mon Sep 17 00:00:00 2001
> From: Alex Kost <alezost@gmail.com>
> Date: Thu, 24 Sep 2015 20:10:29 +0300
> Subject: [PATCH] emacs: devel: Highlight 'modify-phases' keywords.
>
> * emacs/guix-guile.el (guix-guile-keyword-regexp): New function.
> * emacs/guix-devel.el (guix-devel-faces): New custom group.
>   (guix-devel-modify-phases-keyword): New face.
>   (guix-devel-modify-phases-keyword-regexp,
>   guix-devel-font-lock-keywords): New variables.
>   (guix-devel-modify-phases-font-lock-matcher,
>   guix-devel-modify-phases-font-lock-pre): New functions.
>   (guix-devel-mode): Adjust to add/remove font-lock-keywords.

OK!

I also have this one that I find useful:

--8<---------------cut here---------------start------------->8---
;; For Guix g-expressions.
(font-lock-add-keywords 'scheme-mode
			'(("#~" . font-lock-keyword-face)
			  ("#\\$" . font-lock-keyword-face)
			  ("#\\+" . font-lock-keyword-face)))
--8<---------------cut here---------------end--------------->8---

Similarly .dir-locals.el has ‘modify-syntax-entry’ stuff for gexps that
could maybe go to guix-devel.el?

Thanks,
Ludo’.

Re: [PATCH] emacs: devel: Highlight 'modify-phases' keywords.

[Alex Kost]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

Ludovic Courtès (2015-09-25 22:22 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> From ed1c3871b81d4904d106a3ca5aadde2e1803d2a6 Mon Sep 17 00:00:00 2001
>> From: Alex Kost <alezost@gmail.com>
>> Date: Thu, 24 Sep 2015 20:10:29 +0300
>> Subject: [PATCH] emacs: devel: Highlight 'modify-phases' keywords.
>>
>> * emacs/guix-guile.el (guix-guile-keyword-regexp): New function.
>> * emacs/guix-devel.el (guix-devel-faces): New custom group.
>>   (guix-devel-modify-phases-keyword): New face.
>>   (guix-devel-modify-phases-keyword-regexp,
>>   guix-devel-font-lock-keywords): New variables.
>>   (guix-devel-modify-phases-font-lock-matcher,
>>   guix-devel-modify-phases-font-lock-pre): New functions.
>>   (guix-devel-mode): Adjust to add/remove font-lock-keywords.
>
> OK!

Thanks, I have pushed it.

> I also have this one that I find useful:
>
> ;; For Guix g-expressions.
> (font-lock-add-keywords 'scheme-mode
> 			'(("#~" . font-lock-keyword-face)
> 			  ("#\\$" . font-lock-keyword-face)
> 			  ("#\\+" . font-lock-keyword-face)))

Great, what about the attached patch for adding these ones?

> Similarly .dir-locals.el has ‘modify-syntax-entry’ stuff for gexps that
> could maybe go to guix-devel.el?

Yes, they could.  So as you mention it, there is another thing I would
like to add — complex indentation rules.  Specifically, inherited
packages are automatically indented like this:

(package (inherit foo)
         (name "foo")
         ...)

or this:

(package
  (inherit foo)
  (name "foo")
  ...)

while we want it to be:

(package (inherit foo)
  (name "foo")
  ...)

The following code may be used to handle this indentation:


[-- Attachment #2: package-indent.el --]
[-- Type: application/emacs-lisp, Size: 359 bytes --]

[-- Attachment #3: Type: text/plain, Size: 391 bytes --]


But it wouldn't work reliably because of ".dir-locals.el", as
'scheme-indent-function' property will be overridden every time a scheme
file from git repo is visited.  So my proposition is: what about moving
all indentation rules from ".dir-locals.el" to "emacs/guix-devel.el"?
So 'guix-devel-mode' will be responsible for all indenting, highlighting
and other guix-specific things.  WDYT?


[-- Attachment #4: 0001-emacs-devel-Highlight-gexp-symbols.patch --]
[-- Type: text/x-patch, Size: 1630 bytes --]

From 13ebd00178d1f6139da4c1f76a2e358935ce16f0 Mon Sep 17 00:00:00 2001
From: Alex Kost <alezost@gmail.com>
Date: Sat, 26 Sep 2015 22:42:07 +0300
Subject: [PATCH] emacs: devel: Highlight gexp symbols.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Suggested by Ludovic Courtès <ludo@gnu.org>.

* emacs/guix-devel.el (guix-devel-gexp-symbol): New face.
  (guix-devel-font-lock-keywords): Adjust to handle gexp symbols.
---
 emacs/guix-devel.el | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/emacs/guix-devel.el b/emacs/guix-devel.el
index 8a6fc1e..2e85a7e 100644
--- a/emacs/guix-devel.el
+++ b/emacs/guix-devel.el
@@ -43,6 +43,12 @@
   "Face for a `modify-phases' keyword ('delete', 'replace', etc.)."
   :group 'guix-devel-faces)
 
+(defface guix-devel-gexp-symbol
+  '((t :inherit font-lock-keyword-face))
+  "Face for gexp symbols ('#~', '#$', etc.).
+See Info node `(guix) G-Expressions'."
+  :group 'guix-devel-faces)
+
 (defcustom guix-devel-activate-mode t
   "If non-nil, then `guix-devel-mode' is automatically activated
 in Scheme buffers."
@@ -130,7 +136,9 @@ to find 'modify-phases' keywords."
   (save-excursion (up-list) (point)))
 
 (defvar guix-devel-font-lock-keywords
-  `((,(guix-guile-keyword-regexp "modify-phases")
+  `((,(rx (or "#~" "#$" "#$@" "#+" "#+@")) .
+     'guix-devel-gexp-symbol)
+    (,(guix-guile-keyword-regexp "modify-phases")
      (1 'font-lock-keyword-face)
      (guix-devel-modify-phases-font-lock-matcher
       (guix-devel-modify-phases-font-lock-pre)
-- 
2.5.0

Re: [PATCH] emacs: devel: Highlight 'modify-phases' keywords.

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

>> ;; For Guix g-expressions.
>> (font-lock-add-keywords 'scheme-mode
>> 			'(("#~" . font-lock-keyword-face)
>> 			  ("#\\$" . font-lock-keyword-face)
>> 			  ("#\\+" . font-lock-keyword-face)))
>
> Great, what about the attached patch for adding these ones?

Looks good.

>> Similarly .dir-locals.el has ‘modify-syntax-entry’ stuff for gexps that
>> could maybe go to guix-devel.el?
>
> Yes, they could.  So as you mention it, there is another thing I would
> like to add — complex indentation rules.  Specifically, inherited
> packages are automatically indented like this:
>
> (package (inherit foo)
>          (name "foo")
>          ...)
>
> or this:
>
> (package
>   (inherit foo)
>   (name "foo")
>   ...)
>
> while we want it to be:
>
> (package (inherit foo)
>   (name "foo")
>   ...)
>
> The following code may be used to handle this indentation:
>
> (defun guix-devel-package-indent (state indent-point normal-indent)
>   (let ((count (if (and (ignore-errors (down-list) t)
>                         (looking-at "inherit\\>"))
>                    1
>                  0)))
>     (lisp-indent-specform count state indent-point normal-indent)))
>
> (put 'package 'scheme-indent-function 'guix-devel-package-indent)

Nice!  I like it, I think it would be a nice addition.

At the same time I wonder if defining sophisticated indentation rules is
a good idea in general.  (Though this one was already defined anyway.)

> But it wouldn't work reliably because of ".dir-locals.el", as
> 'scheme-indent-function' property will be overridden every time a scheme
> file from git repo is visited.  So my proposition is: what about moving
> all indentation rules from ".dir-locals.el" to "emacs/guix-devel.el"?
> So 'guix-devel-mode' will be responsible for all indenting, highlighting
> and other guix-specific things.  WDYT?

Good question.  .dir-locals.el includes a bunch of internal rules that
do not make sense outside.

For those who do make sense outside, such as rules for ‘package’, I
think having them in .dir-locals.el has the advantage that Emacs users
cannot escape them inadvertently.  With guix-devel.el, there’s a greater
chance of people not loading it.

Unless .dir-locals.el has something like:

   (eval . (load "emacs/guix-devel.el")) ;…

?

Thoughts?

> From 13ebd00178d1f6139da4c1f76a2e358935ce16f0 Mon Sep 17 00:00:00 2001
> From: Alex Kost <alezost@gmail.com>
> Date: Sat, 26 Sep 2015 22:42:07 +0300
> Subject: [PATCH] emacs: devel: Highlight gexp symbols.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Suggested by Ludovic Courtès <ludo@gnu.org>.
>
> * emacs/guix-devel.el (guix-devel-gexp-symbol): New face.
>   (guix-devel-font-lock-keywords): Adjust to handle gexp symbols.

OK.

Thanks,
Ludo’.

.dir-locals.el vs. guix-devel-mode

[Alex Kost]

Ludovic Courtès (2015-09-27 23:29 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
[...]
>> The following code may be used to handle this indentation:
>>
>> (defun guix-devel-package-indent (state indent-point normal-indent)
>>   (let ((count (if (and (ignore-errors (down-list) t)
>>                         (looking-at "inherit\\>"))
>>                    1
>>                  0)))
>>     (lisp-indent-specform count state indent-point normal-indent)))
>>
>> (put 'package 'scheme-indent-function 'guix-devel-package-indent)
>
> Nice!  I like it, I think it would be a nice addition.
>
> At the same time I wonder if defining sophisticated indentation rules is
> a good idea in general.  (Though this one was already defined anyway.)

I agree that it's better to avoid unusual indentation rules.

Actually I don't like that ‘inherit’ field is treated specially, I would
leave it as usual:

(package
  (inherit foo)
  (name "foo")
  ...)

But since the inherited packages have the form:

(package (inherit foo)
  (name "foo")
  ...)

I thought this is an existing rule, so I came up with that indentation
function.

>> But it wouldn't work reliably because of ".dir-locals.el", as
>> 'scheme-indent-function' property will be overridden every time a scheme
>> file from git repo is visited.  So my proposition is: what about moving
>> all indentation rules from ".dir-locals.el" to "emacs/guix-devel.el"?
>> So 'guix-devel-mode' will be responsible for all indenting, highlighting
>> and other guix-specific things.  WDYT?
>
> Good question.  .dir-locals.el includes a bunch of internal rules that
> do not make sense outside.
>
> For those who do make sense outside, such as rules for ‘package’, I
> think having them in .dir-locals.el has the advantage that Emacs users
> cannot escape them inadvertently.  With guix-devel.el, there’s a greater
> chance of people not loading it.
>
> Unless .dir-locals.el has something like:
>
>    (eval . (load "emacs/guix-devel.el")) ;…
>
> ?

I am against this.  At first it is ugly; besides don't forget that it
means "guix-devel.el" will be loaded *each time* you open a scheme file
from the guix git directory.

More generally, I'm strongly against using "eval" in .dir-locals.  IMO
this file should be used only to set local variables, and ideally it
shouldn't evaluate an arbitrary code.

> Thoughts?

So I think .dir-locals.el is not the proper place for indentation rules.
IMHO they should be moved to "guix-devel.el" and the manual should
recommend using 'guix-devel-mode' for editing guix package files as it
provides the proper indenting, highlighting and some useful key
bindings.

-- 
Alex

Re: .dir-locals.el vs. guix-devel-mode

[Taylan Ulrich Bayırlı/Kammer]

Alex Kost <alezost@gmail.com> writes:

> [...]
>
> More generally, I'm strongly against using "eval" in .dir-locals.  IMO
> this file should be used only to set local variables, and ideally it
> shouldn't evaluate an arbitrary code.

Just to clarify: I suppose you mean using eval for things other than
which Emacs can automatically determine to be safe.  E.g.

    (eval . (put 'foo 'scheme-indent-function <n>))

for integer <n> is safe; Emacs will eval that silently if I'm not
mistaken.

But for things where Emacs pesters the user because it finds it
"unsafe," I find that very annoying.

> So I think .dir-locals.el is not the proper place for indentation rules.
> IMHO they should be moved to "guix-devel.el" and the manual should
> recommend using 'guix-devel-mode' for editing guix package files as it
> provides the proper indenting, highlighting and some useful key
> bindings.

IMO anything that can be put into .dir-locals.el without making Emacs
pester the user should be put there.  It's a delight for a Guix newb to
get such settings automatically, because otherwise M-q and TAB will
break existing correct indentation in edited source files, which is very
annoying, and also it might lead to extra patch review round-trips when
the user creates a patch with wrong indentation without noticing.

Just my two cents as a passerby in this thread...

Taylan

Re: .dir-locals.el vs. guix-devel-mode

[Alex Kost]

Taylan Ulrich "Bayırlı/Kammer" (2015-09-28 23:05 +0300) wrote:

> Alex Kost <alezost@gmail.com> writes:
>
>> [...]
>>
>> More generally, I'm strongly against using "eval" in .dir-locals.  IMO
>> this file should be used only to set local variables, and ideally it
>> shouldn't evaluate an arbitrary code.
>
> Just to clarify: I suppose you mean using eval for things other than
> which Emacs can automatically determine to be safe.  E.g.
>
>     (eval . (put 'foo 'scheme-indent-function <n>))
>
> for integer <n> is safe; Emacs will eval that silently if I'm not
> mistaken.

Actually I don't like .dir-locals.el in general.  I consider it a
workaround for the case when there are no appropriate major/minor modes
for editing the project files.

> But for things where Emacs pesters the user because it finds it
> "unsafe," I find that very annoying.

Indeed; it may be even scaring for newbies.  At least it was for me when
I first faced such "unsafe" warning, so I used (setq
enable-local-variables nil) for some time :-)

-- 
Alex

Re: .dir-locals.el vs. guix-devel-mode

[Taylan Ulrich Bayırlı/Kammer]

Alex Kost <alezost@gmail.com> writes:

> Taylan Ulrich "Bayırlı/Kammer" (2015-09-28 23:05 +0300) wrote:
>
>> Alex Kost <alezost@gmail.com> writes:
>>
>>> [...]
>>>
>>> More generally, I'm strongly against using "eval" in .dir-locals.  IMO
>>> this file should be used only to set local variables, and ideally it
>>> shouldn't evaluate an arbitrary code.
>>
>> Just to clarify: I suppose you mean using eval for things other than
>> which Emacs can automatically determine to be safe.  E.g.
>>
>>     (eval . (put 'foo 'scheme-indent-function <n>))
>>
>> for integer <n> is safe; Emacs will eval that silently if I'm not
>> mistaken.
>
> Actually I don't like .dir-locals.el in general.  I consider it a
> workaround for the case when there are no appropriate major/minor modes
> for editing the project files.

I agree a full guix-devel minor mode might be a good idea, but a
passerby contributor or newcomer shouldn't need to know about it IMO.
It will seem like an additional hurdle to them.  If the newcomer sticks
around, they'll find out and use the mode eventually, but first
impressions matter a lot.

For that reason I'd use .dir-locals.el to its limits, i.e. so long as
the user isn't bothered with the unsafe warning.  (So that would
actually mean using it less than how much we use it right now...)

Taylan

Re: .dir-locals.el vs. guix-devel-mode

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> Ludovic Courtès (2015-09-27 23:29 +0300) wrote:
>
>> Alex Kost <alezost@gmail.com> skribis:
>>
> [...]
>>> The following code may be used to handle this indentation:
>>>
>>> (defun guix-devel-package-indent (state indent-point normal-indent)
>>>   (let ((count (if (and (ignore-errors (down-list) t)
>>>                         (looking-at "inherit\\>"))
>>>                    1
>>>                  0)))
>>>     (lisp-indent-specform count state indent-point normal-indent)))
>>>
>>> (put 'package 'scheme-indent-function 'guix-devel-package-indent)
>>
>> Nice!  I like it, I think it would be a nice addition.
>>
>> At the same time I wonder if defining sophisticated indentation rules is
>> a good idea in general.  (Though this one was already defined anyway.)
>
> I agree that it's better to avoid unusual indentation rules.
>
> Actually I don't like that ‘inherit’ field is treated specially, I would
> leave it as usual:

Yeah I ended up doing that quite often lately.

> But since the inherited packages have the form:
>
> (package (inherit foo)
>   (name "foo")
>   ...)
>
> I thought this is an existing rule, so I came up with that indentation
> function.

It is, but until now scheme-mode wouldn’t DTRT.  :-)

>> Good question.  .dir-locals.el includes a bunch of internal rules that
>> do not make sense outside.
>>
>> For those who do make sense outside, such as rules for ‘package’, I
>> think having them in .dir-locals.el has the advantage that Emacs users
>> cannot escape them inadvertently.  With guix-devel.el, there’s a greater
>> chance of people not loading it.
>>
>> Unless .dir-locals.el has something like:
>>
>>    (eval . (load "emacs/guix-devel.el")) ;…
>>
>> ?
>
> I am against this.  At first it is ugly; besides don't forget that it
> means "guix-devel.el" will be loaded *each time* you open a scheme file
> from the guix git directory.
>
> More generally, I'm strongly against using "eval" in .dir-locals.  IMO
> this file should be used only to set local variables, and ideally it
> shouldn't evaluate an arbitrary code.
>
>> Thoughts?
>
> So I think .dir-locals.el is not the proper place for indentation rules.
> IMHO they should be moved to "guix-devel.el" and the manual should
> recommend using 'guix-devel-mode' for editing guix package files as it
> provides the proper indenting, highlighting and some useful key
> bindings.

Yes, but I agree with Taylan: a passerby should get a reasonable setup
in place automatically.  That’s what I like about .dir-locals.el: it
allows you to make sure that a minimum set of rules is in place, which
in turn means that patches are more likely to come out right, which
means less frustration and increased happiness.

Using eval + load in .dir-locals.el is indeed ugly though.  So, for lack
of a better solution, I’m fine having some of the rules duplicated.
Specifically, rules for ‘package’, ‘origin’, ‘operating-system’,
‘substitute*’, ‘with-store’, ‘with-monad’, ‘run-with-store’,
‘run-with-state’, and ‘m…’.

How does that sound?

Ludo’.

Re: .dir-locals.el vs. guix-devel-mode

[Alex Kost]

Ludovic Courtès (2015-09-29 22:16 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
[...]
>> So I think .dir-locals.el is not the proper place for indentation rules.
>> IMHO they should be moved to "guix-devel.el" and the manual should
>> recommend using 'guix-devel-mode' for editing guix package files as it
>> provides the proper indenting, highlighting and some useful key
>> bindings.
>
> Yes, but I agree with Taylan: a passerby should get a reasonable setup
> in place automatically.  That’s what I like about .dir-locals.el: it
> allows you to make sure that a minimum set of rules is in place, which
> in turn means that patches are more likely to come out right, which
> means less frustration and increased happiness.
>
> Using eval + load in .dir-locals.el is indeed ugly though.  So, for lack
> of a better solution, I’m fine having some of the rules duplicated.
> Specifically, rules for ‘package’, ‘origin’, ‘operating-system’,
> ‘substitute*’, ‘with-store’, ‘with-monad’, ‘run-with-store’,
> ‘run-with-state’, and ‘m…’.
>
> How does that sound?

I'd like to have them in "guix-devel.el", but I don't see the reason for
duplicating.  I think they either should be placed in "guix-devel.el"
(surely the right thing for me :-)) or stay in ".dir-locals.el" (I'm
afraid the right thing for the most :-().

Why do you suggest the duplication?

-- 
Ceterum censeo +Carthaginem+ ".dir-locals.el" esse delendam

Re: .dir-locals.el vs. guix-devel-mode

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> Ludovic Courtès (2015-09-29 22:16 +0300) wrote:
>
>> Alex Kost <alezost@gmail.com> skribis:
>>
> [...]
>>> So I think .dir-locals.el is not the proper place for indentation rules.
>>> IMHO they should be moved to "guix-devel.el" and the manual should
>>> recommend using 'guix-devel-mode' for editing guix package files as it
>>> provides the proper indenting, highlighting and some useful key
>>> bindings.
>>
>> Yes, but I agree with Taylan: a passerby should get a reasonable setup
>> in place automatically.  That’s what I like about .dir-locals.el: it
>> allows you to make sure that a minimum set of rules is in place, which
>> in turn means that patches are more likely to come out right, which
>> means less frustration and increased happiness.
>>
>> Using eval + load in .dir-locals.el is indeed ugly though.  So, for lack
>> of a better solution, I’m fine having some of the rules duplicated.
>> Specifically, rules for ‘package’, ‘origin’, ‘operating-system’,
>> ‘substitute*’, ‘with-store’, ‘with-monad’, ‘run-with-store’,
>> ‘run-with-state’, and ‘m…’.
>>
>> How does that sound?
>
> I'd like to have them in "guix-devel.el", but I don't see the reason for
> duplicating.  I think they either should be placed in "guix-devel.el"
> (surely the right thing for me :-)) or stay in ".dir-locals.el" (I'm
> afraid the right thing for the most :-().
>
> Why do you suggest the duplication?

Just to make sure that anyone using Emacs will get things right, even if
they haven’t learned about guix-devel.el yet.

WDYT?

Ludo’.

Re: .dir-locals.el vs. guix-devel-mode

[Mathieu Lirzin]

Alex Kost <alezost@gmail.com> writes:

> Ludovic Courtès (2015-09-29 22:16 +0300) wrote:
>> How does that sound?
>
> I'd like to have them in "guix-devel.el", but I don't see the reason for
> duplicating.  I think they either should be placed in "guix-devel.el"
> (surely the right thing for me :-)) or stay in ".dir-locals.el" (I'm
> afraid the right thing for the most :-().

I prefer to keep them in “.dir-locals.el” too, since it doesn't require
manual configuration from the user.

--
Mathieu Lirzin

Re: .dir-locals.el vs. guix-devel-mode

[Alex Kost]

Ludovic Courtès (2015-10-02 00:34 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> Ludovic Courtès (2015-09-29 22:16 +0300) wrote:
>>
>>> Alex Kost <alezost@gmail.com> skribis:
>>>
>> [...]
>>>> So I think .dir-locals.el is not the proper place for indentation rules.
>>>> IMHO they should be moved to "guix-devel.el" and the manual should
>>>> recommend using 'guix-devel-mode' for editing guix package files as it
>>>> provides the proper indenting, highlighting and some useful key
>>>> bindings.
>>>
>>> Yes, but I agree with Taylan: a passerby should get a reasonable setup
>>> in place automatically.  That’s what I like about .dir-locals.el: it
>>> allows you to make sure that a minimum set of rules is in place, which
>>> in turn means that patches are more likely to come out right, which
>>> means less frustration and increased happiness.
>>>
>>> Using eval + load in .dir-locals.el is indeed ugly though.  So, for lack
>>> of a better solution, I’m fine having some of the rules duplicated.
>>> Specifically, rules for ‘package’, ‘origin’, ‘operating-system’,
>>> ‘substitute*’, ‘with-store’, ‘with-monad’, ‘run-with-store’,
>>> ‘run-with-state’, and ‘m…’.
>>>
>>> How does that sound?
>>
>> I'd like to have them in "guix-devel.el", but I don't see the reason for
>> duplicating.  I think they either should be placed in "guix-devel.el"
>> (surely the right thing for me :-)) or stay in ".dir-locals.el" (I'm
>> afraid the right thing for the most :-().
>>
>> Why do you suggest the duplication?
>
> Just to make sure that anyone using Emacs will get things right, even if
> they haven’t learned about guix-devel.el yet.
>
> WDYT?

I thought that most people (except me) agreed that indentation rules
should stay in ".dir-locals.el", so I don't see a reason why they also
need to be duplicated in "guix-devel.el".

-- 
Alex

Re: .dir-locals.el vs. guix-devel-mode

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> Ludovic Courtès (2015-10-02 00:34 +0300) wrote:
>
>> Alex Kost <alezost@gmail.com> skribis:
>>
>>> Ludovic Courtès (2015-09-29 22:16 +0300) wrote:
>>>
>>>> Alex Kost <alezost@gmail.com> skribis:
>>>>
>>> [...]
>>>>> So I think .dir-locals.el is not the proper place for indentation rules.
>>>>> IMHO they should be moved to "guix-devel.el" and the manual should
>>>>> recommend using 'guix-devel-mode' for editing guix package files as it
>>>>> provides the proper indenting, highlighting and some useful key
>>>>> bindings.
>>>>
>>>> Yes, but I agree with Taylan: a passerby should get a reasonable setup
>>>> in place automatically.  That’s what I like about .dir-locals.el: it
>>>> allows you to make sure that a minimum set of rules is in place, which
>>>> in turn means that patches are more likely to come out right, which
>>>> means less frustration and increased happiness.
>>>>
>>>> Using eval + load in .dir-locals.el is indeed ugly though.  So, for lack
>>>> of a better solution, I’m fine having some of the rules duplicated.
>>>> Specifically, rules for ‘package’, ‘origin’, ‘operating-system’,
>>>> ‘substitute*’, ‘with-store’, ‘with-monad’, ‘run-with-store’,
>>>> ‘run-with-state’, and ‘m…’.
>>>>
>>>> How does that sound?
>>>
>>> I'd like to have them in "guix-devel.el", but I don't see the reason for
>>> duplicating.  I think they either should be placed in "guix-devel.el"
>>> (surely the right thing for me :-)) or stay in ".dir-locals.el" (I'm
>>> afraid the right thing for the most :-().
>>>
>>> Why do you suggest the duplication?
>>
>> Just to make sure that anyone using Emacs will get things right, even if
>> they haven’t learned about guix-devel.el yet.
>>
>> WDYT?
>
> I thought that most people (except me) agreed that indentation rules
> should stay in ".dir-locals.el", so I don't see a reason why they also
> need to be duplicated in "guix-devel.el".

It’s interesting to have them in .dir-locals.el for people who hack Guix
itself.

But it’s also interesting to have them in guix-devel.el for people who
maintain, say, their own package collection outside of the Guix repo.

Does that make sense?

Ludo’.

Re: .dir-locals.el vs. guix-devel-mode

[Alex Kost]

Ludovic Courtès (2015-10-02 15:51 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
[...]
>> I thought that most people (except me) agreed that indentation rules
>> should stay in ".dir-locals.el", so I don't see a reason why they also
>> need to be duplicated in "guix-devel.el".
>
> It’s interesting to have them in .dir-locals.el for people who hack Guix
> itself.
>
> But it’s also interesting to have them in guix-devel.el for people who
> maintain, say, their own package collection outside of the Guix repo.
>
> Does that make sense?

Oh, yes, now it does, thanks for the explanation!  I'll sent the patch
soon.

-- 
Alex

[PATCH] emacs: devel: Add indentation rules.

[Alex Kost]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

Ludovic Courtès (2015-10-02 15:51 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> I thought that most people (except me) agreed that indentation rules
>> should stay in ".dir-locals.el", so I don't see a reason why they also
>> need to be duplicated in "guix-devel.el".
>
> It’s interesting to have them in .dir-locals.el for people who hack Guix
> itself.
>
> But it’s also interesting to have them in guix-devel.el for people who
> maintain, say, their own package collection outside of the Guix repo.

So here it is.  I added indentations only for Guix stuff, as I think
that general Guile things (like 'lambda*' or 'eval-when') shouldn't be
handled by a Guix emacs library.  BTW, all of these Guile things will be
highlighted/indented properly by Geiser 0.8 (see [1], [2]).

[1] https://github.com/jaor/geiser/pull/98
[2] https://github.com/jaor/geiser/pull/102


[-- Attachment #2: 0001-emacs-devel-Add-indentation-rules.patch --]
[-- Type: text/x-patch, Size: 2996 bytes --]

From a878a1fa51cb8072f7af55309ce457d1425841c6 Mon Sep 17 00:00:00 2001
From: Alex Kost <alezost@gmail.com>
Date: Mon, 12 Oct 2015 12:06:32 +0300
Subject: [PATCH] emacs: devel: Add indentation rules.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Suggested by Ludovic Courtès <ludo@gnu.org>.

* emacs/guix-devel.el: Add indentation rules for Guix macros/procedures.
  (guix-devel-scheme-indent): New macro.
  (guix-devel-indent-package): New function.
---
 emacs/guix-devel.el | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/emacs/guix-devel.el b/emacs/guix-devel.el
index b833028..a79b7b1 100644
--- a/emacs/guix-devel.el
+++ b/emacs/guix-devel.el
@@ -24,6 +24,7 @@
 
 ;;; Code:
 
+(require 'lisp-mode)
 (require 'guix-guile)
 (require 'guix-geiser)
 (require 'guix-utils)
@@ -189,6 +190,69 @@ to find 'modify-phases' keywords."
   "A list of `font-lock-keywords' for `guix-devel-mode'.")
 
 \f
+;;; Indentation
+
+(defmacro guix-devel-scheme-indent (&rest rules)
+  "Set `scheme-indent-function' according to rules.
+Each rule should have a form (SYMBOL VALUE).  See `put' for details."
+  (declare (indent 0))
+  `(progn
+     ,@(mapcar (lambda (rule)
+                 `(put ',(car rule) 'scheme-indent-function ,(cadr rule)))
+               rules)))
+
+(defun guix-devel-indent-package (state indent-point normal-indent)
+  "Indentation rule for 'package' form."
+  (let* ((package-eol (line-end-position))
+         (count (if (and (ignore-errors (down-list) t)
+                         (< (point) package-eol)
+                         (looking-at "inherit\\>"))
+                    1
+                  0)))
+    (lisp-indent-specform count state indent-point normal-indent)))
+
+(guix-devel-scheme-indent
+  (bag 0)
+  (build-system 0)
+  (call-with-compressed-output-port 2)
+  (call-with-container 1)
+  (call-with-decompressed-port 2)
+  (call-with-error-handling 0)
+  (container-excursion 1)
+  (emacs-batch-edit-file 1)
+  (emacs-batch-eval 0)
+  (emacs-substitute-sexps 1)
+  (emacs-substitute-variables 1)
+  (file-system 0)
+  (graft 0)
+  (manifest-entry 0)
+  (manifest-pattern 0)
+  (mbegin 1)
+  (mlet 2)
+  (mlet* 2)
+  (modify-phases 1)
+  (munless 1)
+  (mwhen 1)
+  (operating-system 0)
+  (origin 0)
+  (package 'guix-devel-indent-package)
+  (run-with-state 1)
+  (run-with-store 1)
+  (signature-case 1)
+  (substitute* 1)
+  (substitute-keyword-arguments 1)
+  (test-assertm 1)
+  (with-atomic-file-output 1)
+  (with-derivation-narinfo 1)
+  (with-derivation-substitute 2)
+  (with-directory-excursion 1)
+  (with-error-handling 0)
+  (with-monad 1)
+  (with-mutex 1)
+  (with-store 1)
+  (wrap-program 1))
+
+\f
 (defvar guix-devel-keys-map
   (let ((map (make-sparse-keymap)))
     (define-key map (kbd "b") 'guix-devel-build-package-definition)
-- 
2.5.0

Re: [PATCH] emacs: devel: Add indentation rules.

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> Ludovic Courtès (2015-10-02 15:51 +0300) wrote:
>
>> Alex Kost <alezost@gmail.com> skribis:
>>
>>> I thought that most people (except me) agreed that indentation rules
>>> should stay in ".dir-locals.el", so I don't see a reason why they also
>>> need to be duplicated in "guix-devel.el".
>>
>> It’s interesting to have them in .dir-locals.el for people who hack Guix
>> itself.
>>
>> But it’s also interesting to have them in guix-devel.el for people who
>> maintain, say, their own package collection outside of the Guix repo.
>
> So here it is.  I added indentations only for Guix stuff, as I think
> that general Guile things (like 'lambda*' or 'eval-when') shouldn't be
> handled by a Guix emacs library.  BTW, all of these Guile things will be
> highlighted/indented properly by Geiser 0.8 (see [1], [2]).

The future is now!  :-)

> From a878a1fa51cb8072f7af55309ce457d1425841c6 Mon Sep 17 00:00:00 2001
> From: Alex Kost <alezost@gmail.com>
> Date: Mon, 12 Oct 2015 12:06:32 +0300
> Subject: [PATCH] emacs: devel: Add indentation rules.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Suggested by Ludovic Courtès <ludo@gnu.org>.
>
> * emacs/guix-devel.el: Add indentation rules for Guix macros/procedures.
>   (guix-devel-scheme-indent): New macro.
>   (guix-devel-indent-package): New function.

LGTM, thank you!

Ludo’.

Re: [PATCH] emacs: devel: Add indentation rules.

[Alex Kost]

Ludovic Courtès (2015-10-13 12:45 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> Ludovic Courtès (2015-10-02 15:51 +0300) wrote:
>>
>>> Alex Kost <alezost@gmail.com> skribis:
>>>
>>>> I thought that most people (except me) agreed that indentation rules
>>>> should stay in ".dir-locals.el", so I don't see a reason why they also
>>>> need to be duplicated in "guix-devel.el".
>>>
>>> It’s interesting to have them in .dir-locals.el for people who hack Guix
>>> itself.
>>>
>>> But it’s also interesting to have them in guix-devel.el for people who
>>> maintain, say, their own package collection outside of the Guix repo.
>>
>> So here it is.  I added indentations only for Guix stuff, as I think
>> that general Guile things (like 'lambda*' or 'eval-when') shouldn't be
>> handled by a Guix emacs library.  BTW, all of these Guile things will be
>> highlighted/indented properly by Geiser 0.8 (see [1], [2]).
>
> The future is now!  :-)

Indeed, I didn't realize I was writing that message in the same time
when Geiser 0.8 was released.

As we are talking about it, there are a couple of workarounds in the
elisp code for the issues that were solved in Geiser 0.8.  So I would
like to get rid of them and mention in the manual that Geiser 0.8 is
required.  Apparently it shouldn't be done right now as it is too fast.
But how long should we wait?  Would a month be enough time, or maybe
wait until the next Geiser release?

-- 
Alex

Re: [PATCH] emacs: devel: Add indentation rules.

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> As we are talking about it, there are a couple of workarounds in the
> elisp code for the issues that were solved in Geiser 0.8.  So I would
> like to get rid of them and mention in the manual that Geiser 0.8 is
> required.  Apparently it shouldn't be done right now as it is too fast.
> But how long should we wait?  Would a month be enough time, or maybe
> wait until the next Geiser release?

I would wait for at least a couple of releases.  It might be worth
checking which version the major dist^W^W the other major distros ship
before making a decision.

Ludo’.

Re: [PATCH] emacs: devel: Add indentation rules.

[Alex Kost]

Ludovic Courtès (2015-10-14 23:00 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> As we are talking about it, there are a couple of workarounds in the
>> elisp code for the issues that were solved in Geiser 0.8.  So I would
>> like to get rid of them and mention in the manual that Geiser 0.8 is
>> required.  Apparently it shouldn't be done right now as it is too fast.
>> But how long should we wait?  Would a month be enough time, or maybe
>> wait until the next Geiser release?
>
> I would wait for at least a couple of releases.  It might be worth
> checking which version the major dist^W^W the other major distros ship
> before making a decision.

Ouch, it sounds like years, but OK I'll wait, thanks.

-- 
Alex