project--files-in-directory: code formatting

project--files-in-directory: code formatting

[Manuel Uberti]

Hello,

I noticed this piece of code in project.el (line 292 on master):

(command (format "%s %s %s -type f %s -print0"
                 find-program
                 localdir
                 (xref--find-ignores-arguments ignores localdir)
                 (if files
                     (concat (shell-quote-argument "(")
                             " " find-name-arg " "
                             (mapconcat
                              #'shell-quote-argument
                              (split-string files)
                              (concat " -o " find-name-arg " "))
                             " "
                             (shell-quote-argument ")"))"")
                 ))

Woudln't it be cleaner formatted this way?

(command (format "%s %s %s -type f %s -print0"
                 find-program
                 localdir
                 (xref--find-ignores-arguments ignores localdir)
                 (if files
                     (concat (shell-quote-argument "(")
                             " " find-name-arg " "
                             (mapconcat
                              #'shell-quote-argument
                              (split-string files)
                              (concat " -o " find-name-arg " "))
                             " "
                             (shell-quote-argument ")"))
                   "")))

Basically, the "else" of "if files..." on a new line, with the remaining
parenthesis on the same line as this "else".

What do you think?


Since this is not really a bug I didn't open one for it, but I can do it if you
prefer so. Also, I understand this is mostly a matter of personal preferences,
so feel free to ignore this email.


All te best

-- 
Manuel Uberti
www.manueluberti.eu

Re: project--files-in-directory: code formatting

[Stefan Monnier]

> Woudln't it be cleaner formatted this way?
[...]
> Basically, the "else" of "if files..." on a new line, with the remaining
> parenthesis on the same line as this "else".
>
> What do you think?

Yes, please!

I even added a font-lock rule to highlight such cases in red, but
apparently that rule doesn't catch this particular case (if you replace
the empty string with a symbol, it does catch it, tho).


        Stefan

Re: project--files-in-directory: code formatting

[Manuel Uberti]

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

On 04/11/20 15:30, Stefan Monnier wrote
> Yes, please!
> 
> I even added a font-lock rule to highlight such cases in red, but
> apparently that rule doesn't catch this particular case (if you replace
> the empty string with a symbol, it does catch it, tho).
> 

Hi Stefan,

thank you for the prompt reply. I attached a patch for this little change.

Let me know if I did something wrong or you want me to change something.


Thank you

-- 
Manuel Uberti
www.manueluberti.eu

[-- Attachment #2: 0001-Fix-project-files-in-directory-formatting.patch --]
[-- Type: text/x-patch, Size: 1046 bytes --]

From 67f7b611d68131f9dcbedafae93b1d1b4d350908 Mon Sep 17 00:00:00 2001
From: Manuel Uberti <manuel.uberti@inventati.org>
Date: Wed, 4 Nov 2020 15:41:53 +0100
Subject: [PATCH] Fix project--files-in-directory formatting

---
 lisp/progmodes/project.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 8f7482a23d..6c647a092a 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -301,8 +301,8 @@ project--files-in-directory
                                        (split-string files)
                                        (concat " -o " find-name-arg " "))
                                       " "
-                                      (shell-quote-argument ")"))"")
-                          )))
+                                      (shell-quote-argument ")"))
+                            ""))))
     (project--remote-file-names
      (sort (split-string (shell-command-to-string command) "\0" t)
            #'string<))))
-- 
2.25.1

Re: project--files-in-directory: code formatting

[Stefan Monnier]

>> I even added a font-lock rule to highlight such cases in red, but
>> apparently that rule doesn't catch this particular case (if you replace
>> the empty string with a symbol, it does catch it, tho).
> thank you for the prompt reply. I attached a patch for this little change.

Thanks, pushed to `master`.

> Let me know if I did something wrong or you want me to change something.

I tweaked the commit message to better follow our conventions.

But more importantly I was very disappointed that the patch doesn't fix
the font-lock highlighting ;-)


        Stefan

Re: project--files-in-directory: code formatting

[Dmitry Gutov]

On 04.11.2020 15:26, Manuel Uberti wrote:
> Woudln't it be cleaner formatted this way?
> 
> (command (format "%s %s %s -type f %s -print0"
>                   find-program
>                   localdir
>                   (xref--find-ignores-arguments ignores localdir)
>                   (if files
>                       (concat (shell-quote-argument "(")
>                               " " find-name-arg " "
>                               (mapconcat
>                                #'shell-quote-argument
>                                (split-string files)
>                                (concat " -o " find-name-arg " "))
>                               " "
>                               (shell-quote-argument ")"))
>                     "")))
> 
> Basically, the "else" of "if files..." on a new line, with the remaining
> parenthesis on the same line as this "else".
> 
> What do you think?

Thanks!

It was basically a typo (mine), not some actual preference.

Re: project--files-in-directory: code formatting

[Dmitry Gutov]

On 04.11.2020 16:30, Stefan Monnier wrote:
> (if you replace
> the empty string with a symbol, it does catch it, tho)

I tried it with a symbol. Yet, the red failed to show up with my config.

Looking at the text properties, 'help-echo' was there, but not the 
'face' property. Apparently that is because of this personal setup I 
have for dimming parens:

(defface esk-paren-face
   '((((class color) (background light))
      (:foreground "grey55")))
   "Face for parens.")

(dolist (mode '(scheme emacs-lisp lisp lisp-interaction clojure 
clojurescript
                        inferior-emacs-lisp nrepl))
   (when (> (display-color-cells) 8)
     (font-lock-add-keywords (intern (format "%s-mode" mode))
                             '(("(\\|)" . 'esk-paren-face))))
   (add-hook (intern (format "%s-mode-hook" mode)) 'paredit-mode))

One way to avoid the conflict would be to avoid matching parens in 
lisp--match-hidden-arg, but that seems very ad-hoc.

How about this? Fixes the string literal case as well.

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 352210f859..d4411f1fa4 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -477,8 +477,9 @@ lisp--match-confusable-symbol-character
             (1 'font-lock-regexp-grouping-backslash prepend)
             (3 'font-lock-regexp-grouping-construct prepend))
           (lisp--match-hidden-arg
-          (0 '(face font-lock-warning-face
-                    help-echo "Hidden behind deeper element; move to 
another line?")))
+          (0 '( face font-lock-warning-face
+                help-echo "Hidden behind deeper element; move to 
another line?")
+             t))
           (lisp--match-confusable-symbol-character
            0 '(face font-lock-warning-face
                      help-echo "Confusable character"))
@@ -521,8 +522,9 @@ lisp--match-confusable-symbol-character
           (,(concat "(\\(\\(do-\\|with-\\)" lisp-mode-symbol-regexp "\\)")
             (1 font-lock-keyword-face))
           (lisp--match-hidden-arg
-          (0 '(face font-lock-warning-face
-               help-echo "Hidden behind deeper element; move to another 
line?")))
+          (0 '( face font-lock-warning-face
+                help-echo "Hidden behind deeper element; move to 
another line?")
+             t))
           ))
        "Gaudy level highlighting for Lisp modes.")))

Re: project--files-in-directory: code formatting

[Stefan Monnier]

> How about this? Fixes the string literal case as well.

Great minds think alike (tho I used `prepend` instead of `t`).


        Stefan

Re: project--files-in-directory: code formatting

[Dmitry Gutov]

On 04.11.2020 23:20, Stefan Monnier wrote:
> Great minds think alike (tho I used `prepend` instead of `t`)

Even better!

Re: project--files-in-directory: code formatting

[Andrii Kolomoiets]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> I even added a font-lock rule to highlight such cases in red, but
>>> apparently that rule doesn't catch this particular case (if you replace
>>> the empty string with a symbol, it does catch it, tho).
>> thank you for the prompt reply. I attached a patch for this little change.
>
> Thanks, pushed to `master`.

Is it possible to not hightlight text in docstrings?

In 'emacs -Q' put this code into the *scratch* buffer:

(defun foo()
  "
) bar"

"bar" is hightlihgted in red.

Re: project--files-in-directory: code formatting

[Stefan Monnier]

>>>> I even added a font-lock rule to highlight such cases in red, but
>>>> apparently that rule doesn't catch this particular case (if you replace
>>>> the empty string with a symbol, it does catch it, tho).
>>> thank you for the prompt reply. I attached a patch for this little change.
>> Thanks, pushed to `master`.
> Is it possible to not hightlight text in docstrings?

Certainly,


        Stefan "see e8f5657bc7"