Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function

[Jani Nikula <jani@nikula.org>]

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

On Mar 5, 2012 11:11 PM, "Dmitry Kurochkin" <dmitry.kurochkin@gmail.com>
wrote:
>
> On Mon, 5 Mar 2012 22:55:54 +0200, Jani Nikula <jani@nikula.org> wrote:
> > On Mar 5, 2012 5:43 PM, "Dmitry Kurochkin" <dmitry.kurochkin@gmail.com>
> > wrote:
> > >
> > > On Mon, 05 Mar 2012 12:17:43 +0100, Daniel Schoepe <daniel@schoepe.org
>
> > wrote:
> > > > On Mon, 05 Mar 2012 06:21:52 +0400, Dmitry Kurochkin <
> > dmitry.kurochkin@gmail.com> wrote:
> > > > > On Thu,  1 Mar 2012 21:24:38 +0100, Daniel Schoepe <
daniel@schoepe.org>
> > wrote:
> > > > > > notmuch-saved-search-sort-function might destructively modify
its
> > > > > > input (`sort' does that, for instance), so it should not be
given
> > > > > > notmuch-saved-searches directly.
> > > > > > ---
> > > > >
> > > > > -1
> > > > >
> > > > > I think we should require `notmuch-saved-search-sort-function'
not to
> > > > > have side effects.  Current documentation should be more clear
about
> > > > > this.  We need to fix `notmuch-sort-saved-searches' to copy the
list
> > > > > before calling `sort'.  But we should not do it in
> > > > > `notmuch-hello-insert-saved-searches' for any sorting function
(which
> > > > > may not need this copying).
> > > >
> > > > My reasoning was that since sort is such a common function, many
users
> > > > will probably use sort for their own sorting functions, not
realizing
> > > > that it has side effects. This will lead to confusing behavior
that's
> > > > not so easy to track down.
> > > >
> > > > Copying the list of saved searches when running notmuch-hello does
not
> > > > seem be relevant to performance to me, since it's a) not called that
> > > > often and b) the list of saved searches will rarely exceed 30
elements.
> > > >
> > > > Hence, this way we can avoid some headaches for users who define
their
> > > > own sorting functions at a negligible (performance) cost.
Incidentally,
> > > > this is also how notmuch-hello did it before the user-defined
sections
> > > > patches.
> > > >
> > >
> > > I do not buy the argument that we should help users who implement
their
> > > own sorting functions but do not read documentation for functions they
> > > use.  Apparently, those who implemented the `sort' function had
similar
> > > ideas.  And I do not think it is our job to add workarounds for it.
> > >
> > > An alternative (and IMO better) solution would be to allow
customization
> > > of compare function used for sorting instead of the sorting function
> > > itself.
> >
> > Providing the customization of the sort function is more powerful than
the
> > compare function. In the case of saved searches I can imagine people
might
> > want to partially use the original order while sort the rest (e.g.
> > important ones first in predefined order, others sorted).
>
> Valid point.
>
> > In fact this also
> > allows dropping out some elements. And renaming. And changing the
queries...
> >
> > (I had something like that in mind originally but then settled with just
> > capitalizing the important ones to show them first.)
> >
>
> All of these are invalid usages of `notmuch-saved-search-sort-function'.
> The function is meant for sorting only (hence the name).  So the code
> might assume that the function does only sorting.
>
> I do not understand why we need such functionality (renaming,
> capitalizing, etc.).  You can just rename the query itself if you want
> to.  Should be easier IMO.

Just for the record, I have a few important searches capitalized in the
saved searches and just use the regular sort. Capitalized entries sort
before the lowercase ones.

> But if we need such functionality, we should
> not misuse sorting function for it.  We can add `notmuch-saved-searches'
> function which would return saved searches list (sorted, renamed and
> mangled in any other way).  By default it would return
> `notmuch-saved-searches' variable as is.

Agreed.

As to the problem at hand, we should just fix the sort function not to
modify its input. I wouldn't hold my breath waiting for someone to provide
patches for the rest...

BR,
Jani.

[-- Attachment #2: Type: text/html, Size: 5536 bytes --]