Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable concatenation of search queries (split by " / ") #1537

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MaxGyver83
Copy link

@MaxGyver83 MaxGyver83 commented Jul 15, 2020

Implements #1536.

This PR enables the concatenation of two or more search queries. They will be split at ' / ' (a slash surrounded by spaces).

Example:

:search tag:flagged / tag:inbox NOT tag:flagged

This gives you first all flagged e-mails, then all e-mails tagged inbox (not repeating the flagged ones).

Possible problem: I don't know how you would search for a string containing ' / '.

@MaxGyver83 MaxGyver83 changed the title Anable concatenation of search queries (split by " / ") Enable concatenation of search queries (split by " / ") Jul 15, 2020
@MaxGyver83
Copy link
Author

Hi @pazz , I don't think that Travis CI failed because of my PR. The job log says:

./configure: 1558: ./configure: WITH_PYTHON_DOCS: parameter not set

@pazz
Copy link
Owner

pazz commented Jul 17, 2020 via email

@MaxGyver83 MaxGyver83 force-pushed the feature/concatenate-search-queries branch from 7c20949 to c0ca7f0 Compare July 17, 2020 21:19
@MaxGyver83
Copy link
Author

Could you squash the two commits?

Sure, they are squashed now.

@pazz
Copy link
Owner

pazz commented Jul 19, 2020

@lucc: opinions?

@mmartin
Copy link
Contributor

mmartin commented Jul 19, 2020

This is really wonky. It breaks my hooks (search returns double messages or no messages at all) and the result count is wrong.

Copy link
Collaborator

@lucc lucc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add some test for the new query syntax? I can imagine there are some corner cases that are worth testing (see inline comment).

@@ -92,3 +97,6 @@ def _get_next_item(self):

def get_lines(self):
return self.lines

def append(self, iterableWalker):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this is called after somebody started iterating over the IterableWalker already? Is that a case we have to think about? Even if this method is trivial it might be worth it to add a docstring to mention such constrains (if there are some).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I'll try to test that case somehow.

@MaxGyver83
Copy link
Author

This is really wonky. It breaks my hooks (search returns double messages or no messages at all) and the result count is wrong.

@mmartin: Thanks for testing this PR. I have fixed the wrong result count. Could you give an example for a search that returns double messages?

@mmartin
Copy link
Contributor

mmartin commented Jul 20, 2020

@MaxGyver83
Copy link
Author

@MaxGyver83 It's this one: https://github.com/pazz/alot/wiki/Contrib-Hooks#auto-refresh-search-buffer

@mmartin: I could reproduce that behavior. Fixed in 5cf9bdf.

Copy link
Collaborator

@lucc lucc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pazz LGTM. I would be happy if we can have some tests for this. But if you want to push this PR you can "overrule" this request for tests :)

@MaxGyver83
Copy link
Author

I couldn't find any search related tests. Probably because you need real e-mails for that.

@MaxGyver83
Copy link
Author

I don't know if slash is a good query divider. Maybe the word THEN is even better because it matches better the notmuch keywords. For example: search tag:flagged THEN tag:inbox AND NOT tag:flagged.

@pazz
Copy link
Owner

pazz commented Jul 21, 2020

I agree that adding tests should be good.

Conceptually, what bugs me about this series is that it diverges from notmuch's query syntax, at least it looks like it does, which is something that I've so far tried to avoid.

One way to get around that would be not to have a single "meta query" but something like the 'refine' command, which allows to stepwise append result lists for individual, valid notmuch queries.
One could then concatenate such commands to have the same effect as proposed here..

@MaxGyver83
Copy link
Author

One way to get around that would be not to have a single "meta query" but something like the 'refine' command, which allows to stepwise append result lists for individual, valid notmuch queries.
One could then concatenate such commands to have the same effect as proposed here..

Sure, this sounds good to me. If I understand you right, you prefer something like :search tag:flagged; append tag:inbox AND NOT tag:flagged. Or maybe append-search. I'll update my PR. Can you already think of a command name that you like?

@MaxGyver83
Copy link
Author

I started working on the requested change and it looks like it's a big one!

Adding a new command to commands/search.py is easy. But the next logical step would be to replace self.querystring in buffers/search.py:SearchBuffer with a list of querystrings. So the new append command could add a new query to that list and then rebuild the current buffer. But unfortunately the querystring is used in many places, p.e. when checking if there is already an open buffer with the same query or when using the refine command. We need to define a new behavior for these, too. For example, we check instead if there exists already a buffer with the same list of queries and the refine command will only be applied to the last query in that list.

For me it's okay to finish this task but first I want to check if you are okay with such extensive changes to the code. @pazz: What do you think about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants