* [PATCH 0/1] emacs: Pass a copy to notmuch-saved-search-sort-function @ 2012-03-01 14:44 Daniel Schoepe 2012-03-01 14:44 ` [PATCH] " Daniel Schoepe 2012-03-01 20:24 ` [PATCH v2 0/1] " Daniel Schoepe 0 siblings, 2 replies; 13+ messages in thread From: Daniel Schoepe @ 2012-03-01 14:44 UTC (permalink / raw) To: notmuch As Jani discovered, `sort' modifies its input list, leading to deleted saved searches when using notmuch-sort-saved-searches. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-01 14:44 [PATCH 0/1] emacs: Pass a copy to notmuch-saved-search-sort-function Daniel Schoepe @ 2012-03-01 14:44 ` Daniel Schoepe 2012-03-05 20:55 ` [PATCH v2] " Jani Nikula 2012-03-01 20:24 ` [PATCH v2 0/1] " Daniel Schoepe 1 sibling, 1 reply; 13+ messages in thread From: Daniel Schoepe @ 2012-03-01 14:44 UTC (permalink / raw) To: notmuch 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. --- emacs/notmuch-hello.el | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el index aad373d..b2b22df 100644 --- a/emacs/notmuch-hello.el +++ b/emacs/notmuch-hello.el @@ -575,7 +575,10 @@ Complete list of currently available key bindings: (let ((searches (notmuch-hello-query-counts (if notmuch-saved-search-sort-function (funcall notmuch-saved-search-sort-function - notmuch-saved-searches) + ;; Use a copy, since the sorting + ;; function may have side effects, + ;; e.g. if it just `sort's the input. + (copy-seq notmuch-saved-searches)) notmuch-saved-searches) :show-empty-searches notmuch-show-empty-saved-searches)) found-target-pos) -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-01 14:44 ` [PATCH] " Daniel Schoepe @ 2012-03-05 20:55 ` Jani Nikula 2012-03-05 21:08 ` Daniel Schoepe 2012-03-05 21:09 ` Dmitry Kurochkin 0 siblings, 2 replies; 13+ messages in thread From: Jani Nikula @ 2012-03-05 20:55 UTC (permalink / raw) To: Dmitry Kurochkin; +Cc: notmuch [-- Attachment #1: Type: text/plain, Size: 2847 bytes --] 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). 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.) BR, Jani. > > Regards, > Dmitry > > > Cheers, > > Daniel > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch [-- Attachment #2: Type: text/html, Size: 3810 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-05 20:55 ` [PATCH v2] " Jani Nikula @ 2012-03-05 21:08 ` Daniel Schoepe 2012-03-05 21:09 ` Dmitry Kurochkin 1 sibling, 0 replies; 13+ messages in thread From: Daniel Schoepe @ 2012-03-05 21:08 UTC (permalink / raw) To: Jani Nikula, Dmitry Kurochkin; +Cc: notmuch [-- Attachment #1: Type: text/plain, Size: 1220 bytes --] On Mar 5, 2012 5:43 PM, "Dmitry Kurochkin" <dmitry.kurochkin@gmail.com> > 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. Yeah that also sounds a bit more consistent, I guess my perception was colored by my surprise at learning that sort has side effects in elisp. :) On Mon, 5 Mar 2012 22:55:54 +0200, Jani Nikula <jani@nikula.org> wrote: > 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). 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.) I have the same objections to only providing a customization option for comparing. Cheers, Daniel [-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-05 20:55 ` [PATCH v2] " Jani Nikula 2012-03-05 21:08 ` Daniel Schoepe @ 2012-03-05 21:09 ` Dmitry Kurochkin 2012-03-06 8:20 ` Jani Nikula 1 sibling, 1 reply; 13+ messages in thread From: Dmitry Kurochkin @ 2012-03-05 21:09 UTC (permalink / raw) To: Jani Nikula; +Cc: notmuch 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. 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. Regards, Dmitry > BR, > Jani. > > > > > Regards, > > Dmitry > > > > > Cheers, > > > Daniel > > _______________________________________________ > > notmuch mailing list > > notmuch@notmuchmail.org > > http://notmuchmail.org/mailman/listinfo/notmuch Non-text part: text/html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-05 21:09 ` Dmitry Kurochkin @ 2012-03-06 8:20 ` Jani Nikula 0 siblings, 0 replies; 13+ messages in thread From: Jani Nikula @ 2012-03-06 8:20 UTC (permalink / raw) To: Dmitry Kurochkin; +Cc: notmuch [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 0/1] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-01 14:44 [PATCH 0/1] emacs: Pass a copy to notmuch-saved-search-sort-function Daniel Schoepe 2012-03-01 14:44 ` [PATCH] " Daniel Schoepe @ 2012-03-01 20:24 ` Daniel Schoepe 2012-03-01 20:24 ` [PATCH v2] " Daniel Schoepe 1 sibling, 1 reply; 13+ messages in thread From: Daniel Schoepe @ 2012-03-01 20:24 UTC (permalink / raw) To: notmuch I overlooked that copy-seq is from cl.el. This version uses copy-sequence instead. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-01 20:24 ` [PATCH v2 0/1] " Daniel Schoepe @ 2012-03-01 20:24 ` Daniel Schoepe 2012-03-03 21:36 ` Jani Nikula 2012-03-05 2:21 ` Dmitry Kurochkin 0 siblings, 2 replies; 13+ messages in thread From: Daniel Schoepe @ 2012-03-01 20:24 UTC (permalink / raw) To: notmuch 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. --- emacs/notmuch-hello.el | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el index aad373d..e089290 100644 --- a/emacs/notmuch-hello.el +++ b/emacs/notmuch-hello.el @@ -575,7 +575,10 @@ Complete list of currently available key bindings: (let ((searches (notmuch-hello-query-counts (if notmuch-saved-search-sort-function (funcall notmuch-saved-search-sort-function - notmuch-saved-searches) + ;; Use a copy, since the sorting + ;; function may have side effects, + ;; e.g. if it just `sort's the input. + (copy-sequence notmuch-saved-searches)) notmuch-saved-searches) :show-empty-searches notmuch-show-empty-saved-searches)) found-target-pos) -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-01 20:24 ` [PATCH v2] " Daniel Schoepe @ 2012-03-03 21:36 ` Jani Nikula 2012-03-05 2:21 ` Dmitry Kurochkin 1 sibling, 0 replies; 13+ messages in thread From: Jani Nikula @ 2012-03-03 21:36 UTC (permalink / raw) To: Daniel Schoepe, notmuch 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. Hi Daniel, thanks for fixing this. Works for me. Full disclosure: I put the broken sort usage there in the first place... BR, Jani. > --- > emacs/notmuch-hello.el | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el > index aad373d..e089290 100644 > --- a/emacs/notmuch-hello.el > +++ b/emacs/notmuch-hello.el > @@ -575,7 +575,10 @@ Complete list of currently available key bindings: > (let ((searches (notmuch-hello-query-counts > (if notmuch-saved-search-sort-function > (funcall notmuch-saved-search-sort-function > - notmuch-saved-searches) > + ;; Use a copy, since the sorting > + ;; function may have side effects, > + ;; e.g. if it just `sort's the input. > + (copy-sequence notmuch-saved-searches)) > notmuch-saved-searches) > :show-empty-searches notmuch-show-empty-saved-searches)) > found-target-pos) > -- > 1.7.9.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-01 20:24 ` [PATCH v2] " Daniel Schoepe 2012-03-03 21:36 ` Jani Nikula @ 2012-03-05 2:21 ` Dmitry Kurochkin 2012-03-05 11:17 ` Daniel Schoepe 1 sibling, 1 reply; 13+ messages in thread From: Dmitry Kurochkin @ 2012-03-05 2:21 UTC (permalink / raw) To: Daniel Schoepe, notmuch 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). Regards, Dmitry > emacs/notmuch-hello.el | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el > index aad373d..e089290 100644 > --- a/emacs/notmuch-hello.el > +++ b/emacs/notmuch-hello.el > @@ -575,7 +575,10 @@ Complete list of currently available key bindings: > (let ((searches (notmuch-hello-query-counts > (if notmuch-saved-search-sort-function > (funcall notmuch-saved-search-sort-function > - notmuch-saved-searches) > + ;; Use a copy, since the sorting > + ;; function may have side effects, > + ;; e.g. if it just `sort's the input. > + (copy-sequence notmuch-saved-searches)) > notmuch-saved-searches) > :show-empty-searches notmuch-show-empty-saved-searches)) > found-target-pos) > -- > 1.7.9.1 > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-05 2:21 ` Dmitry Kurochkin @ 2012-03-05 11:17 ` Daniel Schoepe 2012-03-05 15:23 ` Tomi Ollila 2012-03-05 15:42 ` Dmitry Kurochkin 0 siblings, 2 replies; 13+ messages in thread From: Daniel Schoepe @ 2012-03-05 11:17 UTC (permalink / raw) To: Dmitry Kurochkin, notmuch [-- Attachment #1: Type: text/plain, Size: 1459 bytes --] 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. Cheers, Daniel [-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-05 11:17 ` Daniel Schoepe @ 2012-03-05 15:23 ` Tomi Ollila 2012-03-05 15:42 ` Dmitry Kurochkin 1 sibling, 0 replies; 13+ messages in thread From: Tomi Ollila @ 2012-03-05 15:23 UTC (permalink / raw) To: Daniel Schoepe, Dmitry Kurochkin, notmuch 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. Hard to say -- maybe the alternative: (defun notmuch-sort-saved-searches (alist) "Generate an alphabetically sorted saved searches alist." - (sort alist (lambda (a b) (string< (car a) (car b))))) + (sort (copy-sequence alist) (lambda (a b) (string< (car a) (car b))))) matches better with the current documentation (of notmuch-saved-search-sort-function). Both sort and copy-sequence are blazingly fast... For more information, read http://www.emacswiki.org/emacs/DestructiveOperations > Cheers, > Daniel Tomi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] emacs: Pass a copy to notmuch-saved-search-sort-function 2012-03-05 11:17 ` Daniel Schoepe 2012-03-05 15:23 ` Tomi Ollila @ 2012-03-05 15:42 ` Dmitry Kurochkin 1 sibling, 0 replies; 13+ messages in thread From: Dmitry Kurochkin @ 2012-03-05 15:42 UTC (permalink / raw) To: Daniel Schoepe, notmuch 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. Regards, Dmitry > Cheers, > Daniel ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-03-06 8:20 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-03-01 14:44 [PATCH 0/1] emacs: Pass a copy to notmuch-saved-search-sort-function Daniel Schoepe 2012-03-01 14:44 ` [PATCH] " Daniel Schoepe 2012-03-05 20:55 ` [PATCH v2] " Jani Nikula 2012-03-05 21:08 ` Daniel Schoepe 2012-03-05 21:09 ` Dmitry Kurochkin 2012-03-06 8:20 ` Jani Nikula 2012-03-01 20:24 ` [PATCH v2 0/1] " Daniel Schoepe 2012-03-01 20:24 ` [PATCH v2] " Daniel Schoepe 2012-03-03 21:36 ` Jani Nikula 2012-03-05 2:21 ` Dmitry Kurochkin 2012-03-05 11:17 ` Daniel Schoepe 2012-03-05 15:23 ` Tomi Ollila 2012-03-05 15:42 ` Dmitry Kurochkin
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).