search

nteract / bookstore

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

Refactor Clone handler #121

Closed mpacer closed 5 years ago

mpacer commented 5 years ago

Closes #109

This does not address the switch in all of the APIs as well, I will add that to this PR as it seems thematically related enough to go with it (and it would run into merge conflicts with this if I were to make it an independent PR).

This introduces BookstoreCloneAPIHandler, that inherits from APIHandler rather than IPythonHandler.

codecov[bot] commented 5 years ago

Codecov Report

Merging #121 into master will decrease coverage by 0.1%. The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   34.83%   34.72%   -0.11%     
==========================================
  Files          11       11              
  Lines         356      360       +4     
==========================================
+ Hits          124      125       +1     
- Misses        232      235       +3
mpacer commented 5 years ago

With 7b46868 this Closes #111

mpacer commented 5 years ago

This is odd, I forgot to update all the URIs in the test_clone.py tests, but they didn't break.

@MSeal That seems concerning, weren't these supposed to be checking that the APIs remained constant?

In particular, I'd have thought test_get_success would fail. Any thoughts as to why it didn't?

mpacer commented 5 years ago

Hrm… this seems to be related to the way that we're mocking the Tornado Application… I think because we're not using any of it's routing logic, this actually doesn't test the APIs themselves, only how the handlers handle arguments

mpacer commented 5 years ago

I think the only outstanding issue is the question around s3_key vs s3_object_key and all of that if you think we should make the change, I'm happy to make it.

It's definitely better to make it before this release than after :)

mpacer commented 5 years ago

oh oops… i shouldn't have changed the api url in the template… that was me just being dumb…I'll push a new PR. I should have trusted the test :/

willingc commented 5 years ago

No need to make a change. Just merge when it is green again. Nice job đź‘Ť

mpacer commented 5 years ago

Yay it's green! and thank you for the guidance — I'm excited to rebase the POST onto this, as I think that will massively improve test coverage.