nteract / bookstore

📚 Notebook storage and publishing workflows for the masses
https://bookstore.readthedocs.io
BSD 3-Clause "New" or "Revised" License
201 stars 23 forks source link

Move async content reading inside s3 client context manager #146

Closed mpacer closed 5 years ago

mpacer commented 5 years ago

This addresses #145 for now. However it does not clarify why the issue arose in the first place.

It is likely that this results in a slightly lower coverage report.

But, it has some advantages:

  1. makes the tests slightly cleaner (since we no longer need to make a class to handle the asynchronous read)
  2. simplifies the API for build_content_model as we now can pass in a string rather than a dict with unused additional fields
codecov[bot] commented 5 years ago

Codecov Report

Merging #146 into master will decrease coverage by 0.28%. The diff coverage is 40%.

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
- Coverage   79.57%   79.29%   -0.29%     
==========================================
  Files          10       10              
  Lines         426      425       -1     
==========================================
- Hits          339      337       -2     
- Misses         87       88       +1
codecov[bot] commented 5 years ago

Codecov Report

Merging #146 into master will decrease coverage by 0.28%. The diff coverage is 40%.

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
- Coverage   79.57%   79.29%   -0.29%     
==========================================
  Files          10       10              
  Lines         426      425       -1     
==========================================
- Hits          339      337       -2     
- Misses         87       88       +1
mpacer commented 5 years ago

@willingc are you comfortable with us merging this? There's not going to be an easy way to avoid dropping code coverage while fixing this bug :/.

MSeal commented 5 years ago

-0.29% for refactoring isn't a problem worth blocking a merge imho