Various improvements #47

rgaudin · 2020-06-16T13:57:35Z

Here's a collection of individual small fixes for identified issues:

Fixed don't require min_chunk_size on Creator #29: using default values on Creator init
Fixed ReadArticle __repr__ is incorrect #37: fixed __repr__ for ReadArticle and File, added on Article
Fixed get_article shouldn't raise RuntimeError #38: raising NotFound on unavailable article.
this might not be wanted as per discussion on get_article shouldn't raise RuntimeError #38 and matthieu's suggestion to leave that to new libzim's. I believe this might be forward compatible but we can strip this out if we think it won't.
Issue get_article shouldn't raise RuntimeError #38: added has_article(url) to test an URL.
Currently uses the NotFound exception but could be changed to use RuntimeError should we opt NotFound out.
Fixed get_metadata() doesn't return a dict #30: get_metadata() now returns a dict of metadata
Didn't receive comment on that issue so I impl. what made sense to me.
Fixed we should accept pathlib.Path #45: Creator now accepts pathlib for filename
1e65ca5 simplifies the public Creator API by refactoring what's used only once and by using better docstrings and annotations. warning: added a ._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.

mgautierfr

There is a lot of few things to change, and some other that may lead to discussion.
It may be better to split the changes in several PR to not block everything while we are discussing only one or two points.

libzim/writer.py

tests/test_libzim.py

libzim/wrapper.pyx

libzim/writer.py

rgaudin · 2020-06-19T22:25:18Z

@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 Creator
test on default params for Creator don't inclue dummy assert 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 keyword
has_article() method have been removed
get_metadata() returns bytes of content of a single metadata article and not the discussed dict.
all references to mandatory metadata are gone
Creator former methods which had been moved inside close() (as used there and once) have now been put inline, still in that close() method.
All docstrings were fixed for the reader API
added __enter__ and __exit__ (does nothing) on reader so it can be used as a context manager (syntax sugar only)

mgautierfr

The final code is good.

However, I would prefer to merge the "review changes" into the other commits. At least :

It is useless to add a default value to the main page and remove it later.
Same for changing the behavior of mandotory_data_ok (or move MANDOTORY_METADATA_KEYS) and remove it.

I think it is pretty difficult to understand correctly the changes when you are exploring the git history to find a bug with this kind of commits in "contradiction".

rgaudin · 2020-06-23T09:30:38Z

OK thanks ; will do soon

rgaudin · 2020-06-23T17:20:14Z

Sorry about the delay, was a bit of a pain to reconcile but it's now clean and ready.

mgautierfr · 2020-06-24T14:24:42Z

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 (5ecfab6)

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.

rgaudin · 2020-06-24T15:00:20Z

Ah! right, don't be sorry for my mistakes. Will do.

raise RuntimeError on out-of-bound get_article_by_id() changed get_article_by_id signature to not use the reserved python keyword `id`

mandatory metadata references gone, as per the spec

Creator converts passed filename to str before passing it to wrapper. This allows transparent use of pathlib.Path Also, Creator.filename now returns a pathlib.Path, created on __init__ from the wrapper returned filename. Tests updated to use pathlib by default and check .filename type. Test ensures passing regular str still works.

- fixed, added or updated docstrings - added type annotations to ease self-discovery - made `mandatory_metadata_ok` a property - added a `_closed` boolean prop in creator to record finalization status - fixed the deletion issue after a contextmanager auto-close (using `self._closed`) - simplify public API by moving single-use method: - `_get_counter_string()` and `write_metadata()` to `close()` - `_update_article_counter()` to `add_article()` - `pascalize()` to `update_metadata()` - `write_metadata()` to autmatically convert Date from datetime.datetime as well

- added types annotations in docstring as cython doesn't forward it to gen code - added a dummy __enter__ and __exit__ so user can use it as a contextmanager for clarity

rgaudin · 2020-06-24T20:52:10Z

@mgautierfr That should be good now. Rebased on master. Tests pass on every single commit.

mgautierfr · 2020-06-25T09:21:27Z

We are good !

rgaudin marked this pull request as draft June 16, 2020 13:57

mgautierfr suggested changes Jun 17, 2020

View reviewed changes

rgaudin force-pushed the rgaudin/various_impr branch from 1e65ca5 to 036e3a7 Compare June 19, 2020 22:12

rgaudin marked this pull request as ready for review June 19, 2020 22:25

rgaudin requested a review from mgautierfr June 19, 2020 22:25

mgautierfr suggested changes Jun 23, 2020

View reviewed changes

rgaudin force-pushed the rgaudin/various_impr branch 2 times, most recently from b062ce6 to b1d3aa3 Compare June 23, 2020 17:16

rgaudin requested a review from mgautierfr June 23, 2020 17:20

rgaudin force-pushed the rgaudin/various_impr branch from b1d3aa3 to 26d98ba Compare June 24, 2020 20:32

renaud gaudin added 7 commits June 24, 2020 20:32

Fixed #29: using default values on Creator init

03ac98b

Fixed #37: fixed __repr__ for ReadArticle and File, added on Article

0080f58

Fixed #38: raising NotFound on unavailable article

70a74b1

raise RuntimeError on out-of-bound get_article_by_id() changed get_article_by_id signature to not use the reserved python keyword `id`

Fixed #30: get_metadata() now returns bytes

2fb30e0

mandatory metadata references gone, as per the spec

Cleaner reader API

52e9e38

- added types annotations in docstring as cython doesn't forward it to gen code - added a dummy __enter__ and __exit__ so user can use it as a contextmanager for clarity

rgaudin force-pushed the rgaudin/various_impr branch from 26d98ba to 52e9e38 Compare June 24, 2020 20:35

mgautierfr approved these changes Jun 25, 2020

View reviewed changes

mgautierfr merged commit 9b20717 into master Jun 25, 2020

mgautierfr deleted the rgaudin/various_impr branch June 25, 2020 09:21

Uh oh!

Various improvements #47

Various improvements #47

Uh oh!

Conversation

rgaudin commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgautierfr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgaudin commented Jun 19, 2020

Uh oh!

mgautierfr left a comment

Choose a reason for hiding this comment

Uh oh!

rgaudin commented Jun 23, 2020

Uh oh!

rgaudin commented Jun 23, 2020

Uh oh!

mgautierfr commented Jun 24, 2020

Uh oh!

rgaudin commented Jun 24, 2020

Uh oh!

rgaudin commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgautierfr commented Jun 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rgaudin commented Jun 16, 2020 •

edited

Loading

rgaudin commented Jun 24, 2020 •

edited

Loading