mirekdlugosz commented 3 years ago

This PR fixes #241 by introducing new after_entry_update_hooks attribute to Reader. I tried following instructions from https://github.com/lemon24/reader/issues/241#issuecomment-859511918 , which were very helpful.

./run.sh typing is fine, but I needed to add some somewhat silly things to existing plugins tests for ./run.sh coverage-all to pass.

Overall, I'm quite happy with current state, but I think documentation leaves few things to be desired.

First, generated documentation looks like that: Screenshot_20210620_005538. For me, signature of content in list actually makes it harder to read. With all "important" and "new in" notices around, this is just too noisy. Also, I can't make versionadded to actually evaluate here.

Second, I'm not sure it's easy to find documentation where it is now. I think part of the problem is that this is the first time we publicly talk about "hooks". So far, documentation only mentioned "plugins", which were documented quite extensively. It's not clear to me whether we want to gradually move from plugins to hooks (as hooks seem to require less code to utilize), should both concepts co-exist, or maybe preferred way is to create plugins that would register callbacks in pre-specified hooks containers.

Please let me know if there is anything you would like me to change before merge, I'll be happy to apply your suggestions.

codecov[bot] commented 3 years ago

Codecov Report

Merging #244 (81eaf41) into master (7a00496) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   94.24%   94.25%   +0.01%     
==========================================
  Files          68       69       +1     
  Lines        8271     8290      +19     
  Branches      869      870       +1     
==========================================
+ Hits         7795     7814      +19     
  Misses        409      409              
  Partials       67       67

Impacted Files	Coverage Δ
src/reader/__init__.py	`100.00% <ø> (ø)`
tests/test_reader_private.py	`100.00% <ø> (ø)`
src/reader/core.py	`100.00% <100.00%> (ø)`
src/reader/plugins/entry_dedupe.py	`100.00% <100.00%> (ø)`
src/reader/plugins/mark_as_read.py	`100.00% <100.00%> (ø)`
src/reader/types.py	`100.00% <100.00%> (ø)`
tests/test_plugins_entry_dedupe.py	`100.00% <100.00%> (ø)`
tests/test_plugins_mark_as_read.py	`100.00% <100.00%> (ø)`
tests/test_reader_hooks.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7a00496...81eaf41. Read the comment docs.

lemon24 commented 3 years ago

This looks great, merged! Thank you for your contribution!

I'll fix any cosmetic details myself, and link the commit to #241. You've saved me a lot of time by doing this :D

Regarding your comments above:

I needed to add some somewhat silly things to existing plugins tests for ./run.sh coverage-all to pass.

Sometimes, # pragma: no cover is acceptable (e.g. for the if status is EntryUpdateStatus.MODIFIED guard; not sure if that was the bit with low coverage), but in general I try to avoid over-using it; you made the right call.

For me, signature of content in list actually makes it harder to read.

Agree, I'll see if there's some Sphinx flag to hide it for Reader alone (for data objects like Entry they're useful, since they prevent having to write the type in the docstring by hand).

With all "important" and "new in" notices around, this is just too noisy.

You read my mind: https://github.com/lemon24/reader/issues/183#issuecomment-862997148 (from 3 days ago). Starting with 2.0, they'll be hidden by a "Changelog" section, like Flask does now.

Also, I can't make versionadded to actually evaluate here.

No worries, Sphinx indentation is trial and error for me as well.

preferred way is to create plugins that would register callbacks in pre-specified hooks containers

This one. Hooks are places where you can customize behavior (in code), plugins are such predefined behavior packaged so it's reusable by others (via configuration).

I think part of the problem is that this is the first time we publicly talk about "hooks".

Yup. I'll add a Hooks user guide section after Plugins when I do another documentation pass.

lemon24 / reader

Fix #241 add after_entry_update_hooks to Reader #244

Codecov Report