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

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

[Basil L. Contovounesios]

dgutov@yandex.ru (Dmitry Gutov) writes:

> branch: master
> commit e0ee41d155b210327eb9c9ad5334f80ed59439f4
> Author: Dmitry Gutov <dgutov@yandex.ru>
> Commit: Dmitry Gutov <dgutov@yandex.ru>
>
>     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

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

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

[Basil L. Contovounesios]

Dmitry Gutov <dgutov@yandex.ru> 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

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

[Basil L. Contovounesios]

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

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Dmitry Gutov <dgutov@yandex.ru> 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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: project.diff --]
[-- Type: text/x-diff, Size: 719 bytes --]

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)

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


Thanks,

-- 
Basil

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?

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

[Basil L. Contovounesios]

Dmitry Gutov <dgutov@yandex.ru> 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

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

[Stefan Monnier]

> 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

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

[Basil L. Contovounesios]

Stefan Monnier <monnier@iro.umontreal.ca> 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