bug#8795: 24.0.50; `completion-try-completion' addition of METADATA arg

bug#8795: 24.0.50; `completion-try-completion' addition of METADATA arg

[Drew Adams]

1. The addition of arg METADATA, without making it optional, breaks
existing code that calls the function with only the first four args.
 
What value of METADATA should be passed to get the equivalent of the
behavior prior to the addition of this arg?  nil?
 
How about making this arg optional, so user code does not need to
special-case Emacs 24 wrt Emacs 23?


2. The METADATA arg is not even described in the doc string.
Users have no way to know how to fix the code this breaks.
Please describe the arg in the doc string.

In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2011-05-31 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.5) --no-opt --cflags
-Ic:/build/include'
 

bug#8795: 24.0.50; `completion-try-completion' addition of METADATA arg

[Drew Adams]

Same problem for `completion-all-completions'.

bug#8795: 24.0.50; `completion-try-completion' addition of METADATA arg

[Stefan Monnier]

> 1. The addition of arg METADATA, without making it optional, breaks
> existing code that calls the function with only the first four args.
 
Indeed, it was meant as an internal function, tho clearly it was never
fully internal since icomplete uses it.

I'll see how I can fix it, but in the mean time, could you tell me how
you use it (I can guess the"where" is "icicles")?


        Stefan

bug#8795: 24.0.50; `completion-try-completion' addition of METADATA arg

[Drew Adams]

> > 1. The addition of arg METADATA, without making it optional, breaks
> > existing code that calls the function with only the first four args.
>  
> Indeed, it was meant as an internal function, tho clearly it was never
> fully internal since icomplete uses it.
> 
> I'll see how I can fix it, but in the mean time, could you tell me how
> you use it (I can guess the "where" is "icicles")?

I use it here:

1. icomplete+.el
http://www.emacswiki.org/emacs/icomplete%2b.el
in the Emacs 23+ definition of `icomplete-completions':

(let* ((mdata  (and (fboundp 'completion--field-metadata)
                    (completion--field-metadata
                     (field-beginning))))
       (most-try
        (if (fboundp 'completion--field-metadata) ; Emacs 24
            (completion-try-completion
             name comps nil (length name) mdata)
          (completion-try-completion
           name comps nil (length name))))
...

2. icicles-fn.el
http://www.emacswiki.org/emacs/icicles-fn.el
in the definitions of `icicle-completion-(all|try)-completion(s)':

(let* ((mdata  (and (fboundp 'completion--field-metadata)
                    (or metadata  (completion--field-metadata
                                   (field-beginning)))))
       (res    (if (fboundp 'completion--field-metadata) ; Emacs 24
                   (completion-all-completions
                    string table pred point mdata)
                 (completion-all-completions
                  string table pred point))))
...

[similarly for `...try...']

Functions `icicle-completion-(all|try)-completion(s)' are used by functions
`icicle-unsorted(-file-name)-prefix-candidates',
`icicle-prefix-any(-file-name)-candidates-p' (in the same file).

Here is a typical use:

(if (icicle-not-basic-prefix-completion-p)
    (icicle-completion-try-completion
     input minibuffer-completion-table
     minibuffer-completion-predicate
     (length input)
     (and (fboundp 'completion--field-metadata)
          (completion--field-metadata ; Emacs 24
           (field-beginning))))
  (try-completion input minibuffer-completion-table
                  minibuffer-completion-predicate))

`icicle-not-basic-prefix-completion-p' returns non-nil if the user chooses to
take advantage of Emacs 23+ `completion-styles' etc.

bug#8795: 24.0.50; `completion-try-completion' addition of METADATA arg

[Stefan Monnier]

> 1. The addition of arg METADATA, without making it optional, breaks
> existing code that calls the function with only the first four args.
 
It's now optional,


        Stefan

bug#8795: 24.0.50; `completion-try-completion' addition of METADATA arg

[Stefan Monnier]

> Still no doc for arg METADATA.

Indeed.  I have no intention to add any doc for it.  It's obvious from
context, and this is an internal function.


        Stefan

bug#8795: 24.0.50; `completion-try-completion' addition of METADATA arg

[Drew Adams]

And if it were really internal then you would not, yourself, need to use it also
in other libraries, such as `icomplete.el'.

And that's the other place where I (need to) use it too: `icomplete.el+', which
tweaks the `icomplete.el' code.

bug#8795: FW: bug#8795: 24.0.50; `completion-try-completion' addition of METADATA arg

[Drew Adams]

> > And it is clearly not internal.
> 
> It is.  "Internal" reflects the intention behind this 
> function.  As the author of the code, I know better than
> you do whether it's internal or not.

Your intention is one thing.  Reality (actual use) is apparently another.

You already acknowledged that you didn't think anyone would ever use this - you
were surprised that people do (have to?).  What you intended as internal, not
realizing the impact of your changes, is not internal - in practice.

Do you think people jump through hoops like this just because they want to
complicate things, with different code for different Emacs versions?

;; Recent Emacs
(completion-try-completion
  input minibuffer-completion-table
  minibuffer-completion-predicate
  (length input)
  (completion--field-metadata (field-beginning)))

instead of just

;; Older Emacs
(try-completion input minibuffer-completion-table
                minibuffer-completion-predicate))

How much simpler it would be to just use `try-completion' here for all Emacs
versions. 

As I said, "If I can easily simplify some of this I would be glad to hear how."
No one _wants_ to use something you intended as internal, if they don't have to.

> > If it were internal then you would not have needed to 
> > change METADATA to `&optional'.  And I was not the only one
> > to point out that its not being optional broke backward compatibility.
> 
> Oh, so because I was nice enough to go through the trouble to preserve
> backward compatibility, I am now bound to additionally document
> the obvious because you don't find it obvious enough?

Backward compatibility takes care of, well, backward compatibility.  It does not
take care of documenting how to use the new (more complex) paraphernalia.

Again, why not?  Is it so hard to explain the meaning and behavior of the
METADATA parameter?  What's wrong with describing even an "internal" function?
If you so strongly resist putting the info in a doc string, then put it in a
code comment, at least.  Code comments are how we document "internal" functions,
no?

Whether something like this is obvious enough is not for you to determine, but
for readers of the code.  An author can vouch for the author's _intention_, but
not for how well the meaning and behavior are communicated.