Full text search (case insensitive) with "-contains"

[empireshades]

Similar to #470 which got stale while waiting for python 2.7 compatibility review, and addresses issue #463 (full text search). This is a pull request to add full title and body (case-insensitive) search functionality using an argument "-S". Structurally similar to the way the tag, date searches work. Searching test has been created as well to verify functionality against the "basic" example journal.

Checklist

• The code change is tested and works locally.
• Tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?

[alichtman]

Can you add some tests to confirm this functionality works as expected? At a glance, this looks good to me, though. It's a pretty clean implementation of the feature.

[empireshades]

Can you add some tests to confirm this functionality works as expected? At a glance, this looks good to me, though. It's a pretty clean implementation of the feature.

Done and thanks!

[wren]

Hi, @empireshades. Thank you for the PR! Thanks for taking the initiative to submit it.

This is looking good. Just a few notes:

• Can you please change the flag to be -contains instead of -search? All of the flags there are searching (dates, tags, etc). So adding a new flag that is named search doesn't seem intuitive to me. A user typing jrnl -from 2019 -contains "foo bar" feels more natural to me.
• Can you please add some tests to make sure this plays nicely with the -and, -or, and -not flags? Your implementation should do just fine, but I like tests. :)
• Can you drop the single-letter flag (-S)? I kinda like it, but all of our other flags are full words, so let's leave it out for now. If we want to add single-letter flags, let's get an issue started to talk about it, and look at it more comprehensively (and make sure all of the flags have single-letter equivalents that don't clash with each other).
• Can you change the use of lower to casefold. lower can't handle other languages very well, and casefold is much more unicode friendly.
• Can you move the search_plan.lower (now contains.casefold) to be defined just before the result list (where tagged and excluded are)? The current implementation would be a bit slower in journals with a lot of entries (since we're executing search_plain.lower twice per entry in the journal). Since the search string doesn't change during execution, there's no reason to keep recalculating it.

[wren]

Here's a great post about the pitfalls of lower, and how to use casefold for better results: https://stackoverflow.com/a/29247821/569146

[wren]

This is really close. I added a few notes for changes.

Thank you!

[empireshades]

Thanks for the feedback @wren. Please see my comments inline below.

• Can you please change the flag to be -contains instead of -search? All of the flags there are searching (dates, tags, etc). So adding a new flag that is named search doesn't seem intuitive to me. A user typing jrnl -from 2019 -contains "foo bar" feels more natural to me.

Will do

• Can you please add some tests to make sure this plays nicely with the -and, -or, and -not flags? Your implementation should do just fine, but I like tests. :)

Will do

• Can you drop the single-letter flag (-S)? I kinda like it, but all of our other flags are full words, so let's leave it out for now. If we want to add single-letter flags, let's get an issue started to talk about it, and look at it more comprehensively (and make sure all of the flags have single-letter equivalents that don't clash with each other).

A search function should be quick and easy to use, something that a single letter (such as "-c"), would help facilitate. I've been using this fork multiple times per day for the past year and if I had to type out a full word (beyond say 4 characters), I definitely would not have picked it up in the first place. I can start an issue for this but also don't want this to get blocked in more discussion. Thoughts?

• Can you change the use of lower to casefold. lower can't handle other languages very well, and casefold is much more unicode friendly.

I'm sold. Will do

• Can you move the search_plan.lower (now contains.casefold) to be defined just before the result list (where tagged and excluded are)? The current implementation would be a bit slower in journals with a lot of entries (since we're executing search_plain.lower twice per entry in the journal). Since the search string doesn't change during execution, there's no reason to keep recalculating it.

Makes sense. Will do.

[wren]

A search function should be quick and easy to use, something that a single letter (such as "-c"), would help facilitate.

Yup, I totally agree. I just want to make sure that we implement for all the switches at the the same time to make sure we have a comprehensive look at the switches (e.g. we have more than one flag that starts with the same letter, etc). In order to make it go quicker, if you want to suggest what each letter should be in the issue itself, it'll expedite the process.

[alichtman]

What about -g for grep? Even though RE isn't supported, I think it may still be intuitive. Not sure. Have to think about it more.

[wren]

@alichtman We can talk about the single line flags in a separate issue.

For the longer flag, I want to keep grep reserved for if/when we implement actual grep. This PR doesn't have most of the functionality that people expect from grep, so I think contains fits best.

[empireshades]

@alichtman We can talk about the single line flags in a separate issue.

For the longer flag, I want to keep grep reserved for if/when we implement actual grep. This PR doesn't have most of the functionality that people expect from grep, so I think contains fits best.

If grep gets implemented, I'd be fine with dropping the 'contains' functionality. I pretty much use this as a 'grep -i' replacement.

[empireshades]

This is really close. I added a few notes for changes.

Thank you!

Please take a look at the updates as I believe I took care of everything you requested except for suggesting single letter flags as a new issue.

[wren]

Hi! I'll take a look at this on Tuesday.

[wren]

Looks great. Thank you! 💯

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.