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

Add option for whole-word search #32

Merged
merged 11 commits into from
Oct 20, 2023

Conversation

bryevdv
Copy link
Contributor

@bryevdv bryevdv commented Oct 12, 2023

This PR adds checkbox option to the search that supports only returning results for whole-word matches.

cc @elliottslaughter @lightsighter


Searching for "64" with the whole-word option unchecked yields many results, including ones with 64 in the middle of words:

Screenshot 2023-10-12 at 12 17 06

Checking the whole-word option updates to fewer results that only have "64" as an entire word:

Screenshot 2023-10-12 at 12 16 41

src/app.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@elliottslaughter elliottslaughter left a comment

Choose a reason for hiding this comment

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

Looks good, just a few things to clean up.

Cargo.toml Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
@bryevdv
Copy link
Contributor Author

bryevdv commented Oct 17, 2023

@elliottslaughter friendly reminder I can't push the button

src/app.rs Outdated
if self.whole_word {
match &self.last_word_regex {
Some(regex) => regex.is_match(s),
_ => false,
Copy link
Contributor

@elliottslaughter elliottslaughter Oct 19, 2023

Choose a reason for hiding this comment

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

This is a matter of taste, but I would write this:

let Some(regex) = &self.last_word_regex else {
  unreachable!();
};
regex.is_match(s)

There are a couple reasons to do this. First, we don't want to just ignore an error here. So the unreachable!() signals this is a branch we shouldn't ever hit (and is an error if we do). Because we know the else branch is unreachable, we can optimize the happy path with let ... else, meaning that the variable regex is always defined subsequent to this statement, and reducing the indentation of the continuation.

Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity, the other way to write this is:

let regex = self.last_word_regex.unwrap();
regex.is_match(s)

That's fine too if you want to write it that way. It's shorter, less explicit, and it's easier to miss the .unwrap() (which panics if the option is not Some(...)). I don't have a strong preference, but I feel the profiler backend suffers a bit from using unwrap() all over the place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ffda863

Definitely better, still acclimating to let Some(...) and matching in general, honestly reminds me (from the mists of memory) of Prolog.

src/app.rs Outdated
self.last_whole_word = self.whole_word;
if self.whole_word {
let regex_string = format!("\\b{}\\b", escape(&self.query));
self.last_word_regex = Some(Regex::new(regex_string.as_str()).unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, forgot to mention: I'm pretty sure regex_string.as_str() can be written &regex_string, because there's an impl somewhere for &String to behave like &str (again with those implied impls).

@elliottslaughter elliottslaughter merged commit 665d9ec into StanfordLegion:master Oct 20, 2023
6 checks passed
@bryevdv bryevdv deleted the bv/word-search branch October 30, 2023 16:33
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.

Whole-word search results
2 participants