Add ZimFileReader and ZimFileArticle implementation and tests #16
Conversation
mgautierfr
left a comment
mgautierfr
left a comment
There was a problem hiding this comment.
Few things to change.
Especially about the blob.
libzim/libzim.pyx
Outdated
| cdef Blob blob = self.c_article.getData(<int> 0) | ||
| data = blob.data()[:blob.size()] | ||
| return data |
There was a problem hiding this comment.
Are you copying the content of the blob or creating a memory view ?
If you are copying the content, you shouldn't.
If you are creating a memory view, you must keep a copy of the blob somewhere to be sure the the content pointed by the blob (and so by the memoryview) is valid.
There was a problem hiding this comment.
Here the code is read to python to return it. What use case do you have in mind to avoid the copy?
tests/test_libzim_file_reader.py
Outdated
| self.zim_test_query = u"Einstein" | ||
|
|
||
| # Zim reader | ||
| self.zim_reader = ZimFileReader(self.zim_file_path) |
There was a problem hiding this comment.
This line alone is not a problem. But it makes me think that it would be better to have a contextmanager for ZimFileReader:
with ZimFileReader.open(path) as zim_reader:
....
There was a problem hiding this comment.
What advantages or use case do you have in mind ? What to put in __enter__() and __exit__() ?
mgautierfr
force-pushed
the
reader
branch
2 times, most recently
from
b6cc1fe to
f9d850b
Compare
The reading part is in the `reader` submodule.
No description provided.