Closed rgaudin closed 4 years ago
@mgautierfr , all your comments have been addressed. There isn't much actual code change beside moving blocks around in the whole PR so I kept it all here.
You can look at individual commits for a quick lookup.
What's changed since last review:
main_page
is now mandatory on Creatorassert True
get_article_by_id()
now raises RuntimeError
on out-of-bound – actually anything but not art.is_good()
, the try block have been removed.get_article_by_id()
's id
param was renamed article_id
to not use reserved keywordhas_article()
method have been removedget_metadata()
returns bytes of content of a single metadata article and not the discussed dict.close()
(as used there and once) have now been put inline, still in that close() method.__enter__
and __exit__
(does nothing) on reader so it can be used as a context manager (syntax sugar only)OK thanks ; will do soon
Sorry about the delay, was a bit of a pain to reconcile but it's now clean and ready.
I'm sorry but it is not totally clean yet :)
Commit ed2267b
(#47) remove the str
in the test, but the acceptance of pathlib as argument come in the commit just after (https://github.com/openzim/python-libzim/pull/47/commits/5ecfab69ffe0586899cbd4ec8a3266c40ea88014)
About the execption, maybe we could return KeyError
and IndexError
instead of NotFound
or RuntimeError
. But we could change this in another PR, this one is long enough.
Ah! right, don't be sorry for my mistakes. Will do.
@mgautierfr That should be good now. Rebased on master. Tests pass on every single commit.
We are good !
Here's a collection of individual small fixes for identified issues:
__repr__
for ReadArticle and File, added on ArticleNotFound
exception but could be changed to useRuntimeError
should we opt NotFound out.._closed
prop to avoid the double-close issue we'd get on exiting the contextmanager and (auto) deletion.Please look at them individually and let me know which would make sense to push as separate PRs.