search

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

Publish test #137

Closed mpacer closed 5 years ago

mpacer commented 5 years ago

This PR grew a bit, however the core of it is the creation of new publish handler tests.

It also refactors some of the code so it is more encapsulated and testable.

Because I realized as I was going through the tests that we weren't doing a great job of catching invalid request bodies, It also adds better validation around those.

That then led to a realization that our API docs were not appropriately specific about the request body. Specifically, it said it needed to be a Contents API compatible message, but that wasn't the case. Furthermore, that behaviour doesn't match the APIs expected in the current notebook's contents handlers.

todo[bot] commented 5 years ago

This is far more lenient than our API docs would suggest. We should reconcile them.

https://github.com/nteract/bookstore/blob/37b748bd3569f759e28ccd4bed1ab9975c19e18d/bookstore/publish.py#L54-L59

This comment was generated by todo based on a TODO comment in 37b748bd3569f759e28ccd4bed1ab9975c19e18d in #137. cc @mpacer.
codecov[bot] commented 5 years ago

Codecov Report

Merging #137 into master will increase coverage by 2.54%. The diff coverage is 51.85%.

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage    68.6%   71.14%   +2.54%     
==========================================
  Files          10       10              
  Lines         395      409      +14     
==========================================
+ Hits          271      291      +20     
+ Misses        124      118       -6
codecov[bot] commented 5 years ago

Codecov Report

Merging #137 into master will increase coverage by 6.27%. The diff coverage is 85.29%.

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage    68.6%   74.87%   +6.27%     
==========================================
  Files          10       10              
  Lines         395      414      +19     
==========================================
+ Hits          271      310      +39     
+ Misses        124      104      -20
willingc commented 5 years ago

Thanks for the changes @mpacer and the explanations. I'm good with the PR as it stands. I will leave for Matthew to review and merge. :sunny:

mpacer commented 5 years ago

Merging, thank you @MSeal and @willingc for your reviews!!