Add searcher add archive #229
Conversation
|
@kelvinhammond Thank you for your PR! Can you please add a small description to the PR? Does it fix a known/tracked issue? |
There was a problem hiding this comment.
@kelvinhammond thank you for your PR.
As @kelson42 wrote, it's important that non-trivial contributions are explained ; justified even in this case as there is no exiting ticket.
As a new hidden feature, expending the README example would also be welcome.
On the PR itself, it can't be merged ATM because the usage is too confusing. As you have probably noticed, getResults() yields entry paths alone. That's fine for single-ZIM search but that's terrible for multi-ZIM ones. How do you expect to use it? Checking each result over the two ZIMs?
So we need to change the API to work differently. This kind of discussion would have been preferred on a ticket but it can happen here.
What changes do you suggest?
| self.c_searcher = move(zim.Searcher(archive.c_archive)) | ||
| def addArchive(self, object archive: Archive) -> Searcher: |
There was a problem hiding this comment.
This doesn't respect our code formatting (casing). Please use add_archive() instead
|
|
||
| class Searcher: | ||
| def __init__(self, archive: Archive) -> None: ... | ||
| def addArchive(self, archive: Archive) -> Searcher: ... |
| if self.download_libzim: | ||
| print("removing downloaded libraries") | ||
| for fpath in self.dylib_file.parent.glob("*.[dylib|so|dll|lib|pc]*"): | ||
| for fpath in self.dylib_file.parent.glob("*.[dylib|so|dll|pc]*"): |
| # download libzim tests | ||
| for url in libzim_urls: | ||
| urlretrieve(url, temp_dir / os.path.basename(url)) # noqa: S310 # nosec | ||
| path = temp_dir / os.path.basename(url) |
There was a problem hiding this comment.
This has nothing to do with this PR. Make a separate one
| @skip_if_offline | ||
| def test_reader_search_multiple_zims(all_zims): | ||
| """Test search across multiple ZIMs""" | ||
| search_count_zimfile = 1 |
There was a problem hiding this comment.
Lacks a comment mentioning the expected query and results so this can be investigated manually the day this will break.
Can probably sit next to the query as well
Yeah, I realized this afterwords and rewrote it in Javascript to use the node-libzim library. |
|
I know the feeling 😣 |
3 participants
No description provided.