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 Ramon's Facet Filter & Search Implementation #32

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

Conversation

ZeroCool2u
Copy link

This PR is a clone of @bloodbare's PR, but rebased on the latest commit from the master branch.

Itt upgrades the tantivy version to 0.14.0 and also upgrades the PyO3 version to 0.14.1.

I haven't yet removed the panic, just wanted to make sure we got this to the point that it's just about the same as the original PR first.

Let me know if it looks okay please!

@fulmicoton
Copy link
Contributor

Reading the review of the original @bloodbare PR, the main blocking point is the panic.

@poljar actually mentioned he would rather have the update of tantivy in a separate PR.

@poljar
Copy link
Contributor

poljar commented Jul 22, 2021

A separate PR would have been preferred but at this point getting this over the finish line is more important.

@ZeroCool2u
Copy link
Author

@poljar @fulmicoton Thanks for the feedback! I was actually trying to update to tantivy 0.15.3, but it looks like that's going to be a bit more work, so I think getting this merged and then working on 0.15 makes sense.

Going to try and figure out the panics now that you're okay with this for the moment.

@ZeroCool2u
Copy link
Author

Also, is something up with the CI pipeline? I just realized it looks like it ran for the original PR, but it doesn't seem to have run for this one. Or has something just not yet triggered it? I did run the tests locally and they all passed, but wanted to check.

@poljar
Copy link
Contributor

poljar commented Jul 22, 2021

Also, is something up with the CI pipeline? I just realized it looks like it ran for the original PR, but it doesn't seem to have run for this one. Or has something just not yet triggered it? I did run the tests locally and they all passed, but wanted to check.

I think Travis (the CI provider) gave us the boot, we'll need to switch to Github Actions.

@@ -277,7 +279,7 @@ impl Index {
#[staticmethod]
fn exists(path: &str) -> PyResult<bool> {
let directory = MmapDirectory::open(path).map_err(to_pyerr)?;
Ok(tv::Index::exists(&directory))
Ok(tv::Index::exists(&directory).unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

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

no unwrap here

Sidhant29 pushed a commit to Sidhant29/tantivy-py that referenced this pull request Feb 1, 2023
…ions/actions/setup-python-4.5.0

build(deps): bump actions/setup-python from 4.2.0 to 4.5.0
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