On 31 August 2017, Yegappan Lakshmanan <[email protected]> wrote:
> Hi,
>
> On Thu, Aug 31, 2017 at 9:02 AM, LCD 47 <[email protected]> wrote:
> > On 27 August 2017, Bram Moolenaar <[email protected]> wrote:
> >>
> >> Patch 8.0.1006
> >> Problem: Cannot parse text with 'erroformat' without changing a quickfix
> >> list.
> >> Solution: Add the "text" argument to getqflist(). (Yegappan Lakshmanan)
> >> Files: runtime/doc/eval.txt, src/evalfunc.c, src/proto/quickfix.pro,
> >> src/quickfix.c, src/testdir/test_quickfix.vim
> >
> > It's awesome that this feature has been added, however the interface
> > is confusing:
> >
> > [...]
> >> + text use 'errorformat' to extract items from the
> >> + text and return the resulting entries. The
> >> + value can be a string with one line or a list
> >> + with multiple lines. The current quickfix
> >> list
> >> + is not modified.
> > [...]
> >> + When 'text' is specified, all the other items are ignored.
> >> The
> >> + returned dictionary contains the entry 'items' with the list
> >> + of entries.
> >> In case of error processing {what}, an empty dictionary is
> >> returned.
> > [...]
> >
> > What is the point in reusing getqflist() for this instead of
> > writing a new function? It makes no sense: when "text" is used
> > the other arguments are silently ignored, and the {"items": ...}
> > around the qflist is useless. Why not a function getexpr(list),
> > that returns a list?
> >
>
> It made sense to use the getqflist() function to either return an
> existing list from the quickfix stack or a new list from the supplied
> text.
This might well be, but the end user it still doesn't make sense.
When "text" is usedi all other arguments are ignored, and the result is
always a new list. The use case of getqflist() with "text" genuinely
has almost nothing in common with calling it without "text".
> It also avoids adding many functions for dealing with the quickfix
> lists.
Then perhaps a number of functions could have been refactored. I do
understand that this can break things and can result in a much bigger
mess, but as I said, from the point of view of the end user what came
out was is than great. _shrug_
> > The return value is inconsistent: getqflist("text": []) returns {}
> > rather than {"items": []}, which is indistinguishable from an error.
> >
>
> The attached patch fixes this problem.
That's good to hear, thank you.
> > The fact that unknown keys in the input dictionary are silently
> > ignored means among other things that there is no way to test for
> > the new feature. Compare to:
> >
>
> You can check for a particular patch [has("patch-8.0.1006")].
[...]
Yes, but that has its own set of problems, one of the main ones
being that this kind of tests mean nothing with Neovim.
On 31 August 2017, Bram Moolenaar <[email protected]> wrote:
> As Yegappan mentioned, using a separate function means a lot of the
> arguments need to be duplicated. It's not idea, but considering these
> functions will be used in scripts and won't be typed it's not that
> important.
Well, it isn't a show stopper by any means, but it's still confusing
and really, really ugly. A good API should make things easy for the end
user, rather than for people writing the API. Just saying.
> > The return value is inconsistent: getqflist("text": []) returns {}
> > rather than {"items": []}, which is indistinguishable from an error.
> >
> > The fact that unknown keys in the input dictionary are silently
> > ignored means among other things that there is no way to test for the
> > new feature. Compare to:
> >
> > if exists("*getexpr")
> > let qflist = getexpr(['some input'])
> > else
> > " do it the old way
> > endif
> >
> > or
> >
> > try
> > let qflist = getexpr(['some input'])
> > catch /\m^Vim\%((\a\+)\)\=:E117/
> > " do it the old way
> > endif
>
> You can come up with a call that would return a known result, if it
> instead returns an empty list you know it doesn't work. You won't
> have another way to get the functionality, thus you would probably
> throw an error then.
This is the "autoconf" way. There's a reason why there are maybe 3
people out there who actually know how to write autoconf code, and even
those are probably hating it. :)
> > Finally, there's also a confusion about "text" being a key on
> > input, and also a key with a completely different meaning on output.
>
> "text" is indeed very generic. We could always require passing a list
> (one item would be the same as passing a string), and then the item
> could be called "lines". Would that work better?
Absolutely. That would actually make sense, and it would also allow
testing the feature.
> > Seriously: did anybody actually stop to think for 5 minutes
> > about the user interface of this function?
>
> The patch has been open for comments for quite a while.
Sorry, I'm not trying to point fingers. My personal history with
posting feedback here has been less than stellar, which is why I haven't
been following the project that closely in recent time. Then again,
perhaps no feedback at all might be better than late or occasional
feedback. _shrug_
/lcd
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
