search

openzim / python-libzim

Libzim binding for Python: read/write ZIM files in Python
https://pypi.org/project/libzim/
GNU General Public License v3.0
62 stars 20 forks source link

Add context manager version of ZimCreator to enforce automatic finalization #18

Closed pirate closed 4 years ago

pirate commented 4 years ago

Usage:

from libzim import zimcreator, ZimArticle...

with zimcreator(...) as zc:
    zc.add(ZimArticle(...))

# zc.finalize() is called automatically on leaving the context
kelson42 commented 4 years ago

@pirate I have added #14 as linked issue, I guess this is the issue which is fixed here. Thank you very much for finally have made the proper fix in place of just fixing the test. Really much appreciated.

rgaudin commented 4 years ago

@kelson42 I'm not sure this issue will be fixed by this PR. This is an additional convenient way of using the creator that calls finalize() for you but:

mgautierfr commented 4 years ago

It is syntaxic sugar, but I prefer to have a classmethod open as contentmanager (move the function as a classmethod open in ZimCreator):

with Zimcreator.open(...) as zc:
    zc.add(ZimArticle(...))

or directly with to constructor (implement __enter__ and __exit__ in ZimCreator :

with Zimcreator(....) as zc:
    ...

you can't always use a context manager (especially in our current scrapers)

I'm curious to see why.

so it's user's responsibility to call it to get expected behavior

Yes, if you don't use a context manager you have to close/finalize the object yourself. It is like for a plain file object open to write in it. If you don't close the file and quit your program, you may have data not written to the fs.

but are there side consequences in not doing so that should be prevented?

For now, if you don't call finalize before destroying the creator, you have a sigsev fault in libzim.

mgautierfr commented 4 years ago

Please rebase this PR on master.

jdcaballerov commented 4 years ago

Suggestions implemented based of master in https://github.com/openzim/python-libzim/pull/24