Quoth Mark Walters on Oct 23 at 10:56 am: > > Hi > > On Wed, 23 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote: > > Quoth Mark Walters on Oct 22 at 10:43 pm: > >> This looks good to me +1. It makes the code clearer and nicer to read as > >> well as giving a better user experience, and it is makes fixing the long > >> standing tagging races simpler. > >> > >> I have a couple of docstring comments: > >> > >> In patch 2 perhaps notmuch-tag-completions could have a docstring. > > > > Added. I noticed that I had failed to update the call from > > `notmuch-select-tag-with-completion', so I fixed that, too. I don't > > understand why we take lists of search terms in random places and > > never use more than one element, but I suppose this series doesn't > > make that any worse. > > As far as I can see, at the end of the series, notmuch-tag-completions > is only called with no argument: i.e., it's always just finding the list > of all tags. This is because notmuch-select-tag-with-completion is only > called once from "notmuch-search-filter-by-tag" with no search-terms > argument. So it might be nice to just remove the search-terms > completely. (The only downside is we might break user lisp.) That's true. Though it doesn't significantly complicate the code and it's hard to say if we may need this for something else in the future, so I'd just as soon leave it until we have a more compelling reason to remove it (e.g., if we add tag list caching or something). > Best wishes > > Mark > > > >> In Patch 4 I think the docstring for notmuch-search-tag is outdated: it > >> is "Change tags for the currently selected thread or region." but > >> beg and end can now be specified by the caller. > > > > I've left the first sentence as it is, since it's good interactive > > documentation and a typical way to describe functions even if they > > take a region as arguments (see, for example, `kill-region'). But > > I've elaborated the rest of the docstring to be clearer about this. > > > >> and one actual comment: > >> > >> in patch 3 (for show) delete-dups is called before the list is passed to > >> notmuch-read-tag-changes whereas it is not for search or pick. > >> Obviously this is not actually a problem but it might be worth being > >> consistent. > > > > Ah, whoops. I'd done this before I decided to handle duplicates in > > `notmuch-read-tag-changes'. Since it's redundant, I've removed it. > > > >> But that was all I found. All tests pass and everything I try behaves > >> exactly as expected. > >> > >> Best wishes > >> > >> Mark > >> > >> > >> On Tue, 22 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote: > >> > This series improves tag change completion in various ways for > >> > commands like +, -, and *. > >> > > >> > From a user perspective, this provides command-specific prompts like > >> > "Tag message" and "Tag all" instead of the generic "Tag" prompt, and > >> > bases tag removal completions on the tags that are in the buffer, > >> > rather than the current tags in the database, providing a more > >> > predicable experience. > >> > > >> > From an implementation perspective, this new tag removal completion > >> > behavior improves efficiency and eliminates a road block to fixing the > >> > tagging race bug (which otherwise results in massive queries just to > >> > compute removal completions). The new code is also more "Elispy" and > >> > predictable because all tag change prompting now occurs at the > >> > interactive entry points, rather than buried under several layers of > >> > non-interactive calls. > >> > > >> > This is a spiritual successor to > >> > id:1354263691-19715-1-git-send-email-markwalters1009@gmail.com, though > >> > it takes a very different approach. This is also a prerequisite to > >> > the tag race fix in > >> > id:1381185201-25197-1-git-send-email-amdragon@mit.edu and I plan to > >> > send an updated version of that series when this one is accepted. > >> > > >> > Patches 1, 5, and 6 could be pushed on their own. They fix bugs or > >> > sort of bugs that get in the way of the rest of the series.