Re: [Emacs-diffs] master e0ee41d: Allow customizing the display of project file names when reading

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [Emacs-diffs] master e0ee41d: Allow customizing the display of project file names when reading

Basil L. Contovounesios
[hidden email] (Dmitry Gutov) writes:

> branch: master
> commit e0ee41d155b210327eb9c9ad5334f80ed59439f4
> Author: Dmitry Gutov <[hidden email]>
> Commit: Dmitry Gutov <[hidden email]>
>
>     Allow customizing the display of project file names when reading
>    
>     To hopefully resolve a long-running discussion
>     (https://lists.gnu.org/archive/html/emacs-devel/2019-05/msg00162.html).
>    
>     * lisp/progmodes/project.el (project-read-file-name-function):
>     New variable.
>     (project--read-file-absolute, project--read-file-cpd-relative):
>     New functions, possible values for the above.
>     (project-find-file-in): Use the introduced variable.
>     (project--completing-read-strict): Retain just the logic that fits
>     the name.

[...]

> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index 7c8ca15..ddb4f33 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -470,55 +464,72 @@ recognized."
>                  (project-external-roots pr))))
>      (project-find-file-in (thing-at-point 'filename) dirs pr)))
>  
> +(defcustom project-read-file-name-function #'project--read-file-cpd-relative
> +  "Function to call to read a file name from a list.
> +For the arguments list, see `project--read-file-cpd-relative'."
> +  :type '(repeat (choice (const :tag "Read with completion from relative names"
> +                                project--read-file-cpd-relative)
> +                         (const :tag "Read with completion from absolute names"
> +                                project--read-file-absolute)
> +                         (function :tag "custom function" nil))))
> +
> +(defun project--read-file-cpd-relative (prompt
> +                                        all-files &optional predicate
> +                                        hist default)
> +  (let* ((common-parent-directory
> +          (let ((common-prefix (try-completion "" all-files)))
> +            (if (> (length common-prefix) 0)
> +                (file-name-directory common-prefix))))
> +         (cpd-length (length common-parent-directory))
> +         (prompt (if (zerop cpd-length)
> +                     prompt
> +                   (concat prompt (format " in %s" common-parent-directory))))
> +         (substrings (mapcar (lambda (s) (substring s cpd-length)) all-files))
> +         (new-collection (project--file-completion-table substrings))
> +         (res (project--completing-read-strict prompt
> +                                               new-collection
> +                                               predicate
> +                                               hist default)))
> +    (concat common-parent-directory res)))

Sorry if this has been discussed before, and maybe it's just me, but I
don't think it makes sense to offer internal double hyphen functions as
alternatives for a user option, let alone to point the user to such an
internal function which lacks a docstring, "for the argument list".

Thanks,

--
Basil

Reply | Threaded
Open this post in threaded view
|

Re: [Emacs-diffs] master e0ee41d: Allow customizing the display of project file names when reading

Dmitry Gutov
On 15.05.2019 20:46, Basil L. Contovounesios wrote:
> Sorry if this has been discussed before, and maybe it's just me, but I
> don't think it makes sense to offer internal double hyphen functions as
> alternatives for a user option, let alone to point the user to such an
> internal function which lacks a docstring, "for the argument list".

No, we never discussed this in particular. So I pushed the cleanest
version that made sense to me.

Do we not do that (put internal functions as possible values of a user
option)?

Reply | Threaded
Open this post in threaded view
|

Re: [Emacs-diffs] master e0ee41d: Allow customizing the display of project file names when reading

Stefan Monnier
In reply to this post by Basil L. Contovounesios
> Sorry if this has been discussed before, and maybe it's just me, but I
> don't think it makes sense to offer internal double hyphen functions as
> alternatives for a user option,

I think I should plead guilty, here.  Yet, I also think I agree with you.
Should I split in half?


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: [Emacs-diffs] master e0ee41d: Allow customizing the display of project file names when reading

Basil L. Contovounesios
In reply to this post by Dmitry Gutov
Dmitry Gutov <[hidden email]> writes:

> On 15.05.2019 20:46, Basil L. Contovounesios wrote:
>> Sorry if this has been discussed before, and maybe it's just me, but I
>> don't think it makes sense to offer internal double hyphen functions as
>> alternatives for a user option, let alone to point the user to such an
>> internal function which lacks a docstring, "for the argument list".
>
> No, we never discussed this in particular. So I pushed the cleanest version that
> made sense to me.

I'm grateful for the addition, but I think user options ought to also
make sense to the user.

> Do we not do that (put internal functions as possible values of a user option)?

Sorry, the emphasis in my last message was meant to be on the lack of
any description of the user option's arglist.  If the defcustom is to
refer the user elsewhere, that place might at least have a docstring.

Though I don't understand why the user would be presented with internal
functions as options (it makes it harder to distinguish internal from
external definitions and APIs), I don't feel strongly enough about it to
argue against it.

Thanks,

--
Basil

Reply | Threaded
Open this post in threaded view
|

Re: [Emacs-diffs] master e0ee41d: Allow customizing the display of project file names when reading

Basil L. Contovounesios
In reply to this post by Stefan Monnier
Stefan Monnier <[hidden email]> writes:

>> Sorry if this has been discussed before, and maybe it's just me, but I
>> don't think it makes sense to offer internal double hyphen functions as
>> alternatives for a user option,
>
> I think I should plead guilty, here.  Yet, I also think I agree with you.
> Should I split in half?

That sounds painful, but a world with two Stefans sounds interesting to
say the least, so I'm split on this.

--
Basil

Reply | Threaded
Open this post in threaded view
|

Re: [Emacs-diffs] master e0ee41d: Allow customizing the display of project file names when reading

Basil L. Contovounesios
In reply to this post by Basil L. Contovounesios
"Basil L. Contovounesios" <[hidden email]> writes:

> Dmitry Gutov <[hidden email]> writes:
>
>> On 15.05.2019 20:46, Basil L. Contovounesios wrote:
>>> Sorry if this has been discussed before, and maybe it's just me, but I
>>> don't think it makes sense to offer internal double hyphen functions as
>>> alternatives for a user option, let alone to point the user to such an
>>> internal function which lacks a docstring, "for the argument list".
>>
>> No, we never discussed this in particular. So I pushed the cleanest version that
>> made sense to me.
>
> I'm grateful for the addition, but I think user options ought to also
> make sense to the user.
>
>> Do we not do that (put internal functions as possible values of a user option)?
>
> Sorry, the emphasis in my last message was meant to be on the lack of
> any description of the user option's arglist.  If the defcustom is to
> refer the user elsewhere, that place might at least have a docstring.
I think even something as simple as the following would help:


diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index eab508af3a..cc45a71f57 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -477,6 +477,10 @@ project-read-file-name-function
 (defun project--read-file-cpd-relative (prompt
                                         all-files &optional predicate
                                         hist default)
+  "Read a file name, prompting with PROMPT.
+ALL-FILES is a list of possible file name completions.
+PREDICATE, HIST, and DEFAULT have the same meaning as in
+`completing-read'."
   (let* ((common-parent-directory
           (let ((common-prefix (try-completion "" all-files)))
             (if (> (length common-prefix) 0)


Thanks,

--
Basil
Reply | Threaded
Open this post in threaded view
|

Re: [Emacs-diffs] master e0ee41d: Allow customizing the display of project file names when reading

Dmitry Gutov
On 16.05.2019 12:51, Basil L. Contovounesios wrote:
> I think even something as simple as the following would help:

LGTM, thanks you.

Do you have commit access?

Reply | Threaded
Open this post in threaded view
|

Re: [Emacs-diffs] master e0ee41d: Allow customizing the display of project file names when reading

Basil L. Contovounesios
Dmitry Gutov <[hidden email]> writes:

> On 16.05.2019 12:51, Basil L. Contovounesios wrote:
>> I think even something as simple as the following would help:
>
> LGTM, thanks you.
>
> Do you have commit access?

Yes, pushed to master[1].

[1: b2c0eb63dd]: Add docstring to project--read-file-cpd-relative
  2019-05-16 23:26:27 +0100
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b2c0eb63dd1f0d68de9bf7f14cc337df51617dcc

Thanks,

--
Basil