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

feat: Backward search api #457

Merged
merged 8 commits into from Oct 19, 2021
Merged

Conversation

rob-p
Copy link
Contributor

@rob-p rob-p commented Oct 8, 2021

Improve the backward_search API to inform the user if there was a complete match, a partial (suffix) match, or no match during the search.

@coveralls
Copy link

coveralls commented Oct 13, 2021

Coverage Status

Coverage decreased (-0.03%) to 88.106% when pulling 91c540c on rob-p:backward_search-api into 43d198e on rust-bio:master.

@rob-p
Copy link
Contributor Author

rob-p commented Oct 14, 2021

There were some issues that came up because I didn't modify the examples in the docs to use the new result type. I've fixed those now and cargo test comes up clean locally.

@johanneskoester
Copy link
Contributor

May I ask you to format with the latest stable rust version?

@rob-p
Copy link
Contributor Author

rob-p commented Oct 14, 2021

Done!

Comment on lines +647 to +654
let positions = match sai {
BackwardSearchResult::Complete(saint) => saint.occ(&sa),
BackwardSearchResult::Partial(saint, l) => {
partial_match_len = l;
saint.occ(&sa)
}
BackwardSearchResult::Absent => Vec::<usize>::new(),
};
Copy link
Member

Choose a reason for hiding this comment

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

Since we're expecting a partial match here, I think having only the corresponding Partial match arm and a panic/unreachable/... for the other two should make this (and also the other) testcase(s) clearer

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, github showed this too late to me (after merging actually). Can you (when you find the time), modify this in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Sure

@johanneskoester johanneskoester merged commit eb9d378 into rust-bio:master Oct 19, 2021
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.

None yet

5 participants