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

Add PR #75 review feedback for python files from @MSeal and @willingc #90

Closed willingc closed 5 years ago

willingc commented 5 years ago
todo[bot] commented 5 years ago

refactor the import, PACKAGE_DIR, del

https://github.com/nteract/bookstore/blob/5eba02bf1b5efb616b0aa8cd62e756bec688a642/bookstore/__init__.py#L1-L6

This comment was generated by todo based on a TODO comment in 5eba02bf1b5efb616b0aa8cd62e756bec688a642 in #90. cc @willingc.
todo[bot] commented 5 years ago

should this revert to not using regex ?

https://github.com/nteract/bookstore/blob/5eba02bf1b5efb616b0aa8cd62e756bec688a642/bookstore/client/nb_client.py#L33-L36

This comment was generated by todo based on a TODO comment in 5eba02bf1b5efb616b0aa8cd62e756bec688a642 in #90. cc @willingc.
todo[bot] commented 5 years ago

determine if extract kernel id is used more than once

https://github.com/nteract/bookstore/blob/5eba02bf1b5efb616b0aa8cd62e756bec688a642/bookstore/client/nb_client.py#L222-L223

This comment was generated by todo based on a TODO comment in 5eba02bf1b5efb616b0aa8cd62e756bec688a642 in #90. cc @willingc.
todo[bot] commented 5 years ago

refactor to /api/bookstore/publish/ ?

https://github.com/nteract/bookstore/blob/5eba02bf1b5efb616b0aa8cd62e756bec688a642/bookstore/client/store_client.py#L17-L20

This comment was generated by todo based on a TODO comment in 5eba02bf1b5efb616b0aa8cd62e756bec688a642 in #90. cc @willingc.
todo[bot] commented 5 years ago

refactor to /api/bookstore/clone/ ?

https://github.com/nteract/bookstore/blob/5eba02bf1b5efb616b0aa8cd62e756bec688a642/bookstore/client/store_client.py#L35-L40

This comment was generated by todo based on a TODO comment in 5eba02bf1b5efb616b0aa8cd62e756bec688a642 in #90. cc @willingc.
todo[bot] commented 5 years ago

bookstore settings review https://github.com/nteract/bookstore/pull/75/files?file-filters%5B%5D=.py#r281830978

https://github.com/nteract/bookstore/blob/5eba02bf1b5efb616b0aa8cd62e756bec688a642/bookstore/clone.py#L31-L34

This comment was generated by todo based on a TODO comment in 5eba02bf1b5efb616b0aa8cd62e756bec688a642 in #90. cc @willingc.
todo[bot] commented 5 years ago

Fail fast here if no s3 key

https://github.com/nteract/bookstore/blob/5eba02bf1b5efb616b0aa8cd62e756bec688a642/bookstore/clone.py#L92-L97

This comment was generated by todo based on a TODO comment in 5eba02bf1b5efb616b0aa8cd62e756bec688a642 in #90. cc @willingc.
mpacer commented 5 years ago

@MSeal re: your comments on testing.

I would love your insight on how to test the various clients. I am super enthusiastic about that just had a tricky time getting that to start as to do it properly requires some pretty extensive integration test work.

Regarding the cloning functionality itself that was introduced in #75, that was what the js based integration tests were testing.

MSeal commented 5 years ago

@mpacer I would argue we need unittests first here most of all. You'll need to mock s3 and web interfaces above and below the various functions, but there's a lot of logic here for handling edge cases that need coverage for reliable development. A starting place would be to get a test for clone get receiving and empty file path. I can help set up first first couple tests when I get back if it'd help get the ball rolling.

todo[bot] commented 5 years ago

bookstore settings review https://github.com/nteract/bookstore/pull/75/files?file-filters%5B%5D=.py#r281830978

https://github.com/nteract/bookstore/blob/f1b45689591823169cdd1dbc36e94f3d4afef58c/bookstore/clone.py#L55-L58

This comment was generated by todo based on a TODO comment in f1b45689591823169cdd1dbc36e94f3d4afef58c in #90. cc @willingc.
todo[bot] commented 5 years ago

bookstore settings review https://github.com/nteract/bookstore/pull/75/files?file-filters%5B%5D=.py#r281830978

https://github.com/nteract/bookstore/blob/8bb2fb74aaac861b7454ed4710d6b6d2eb0ca0c4/bookstore/clone.py#L55-L58

This comment was generated by todo based on a TODO comment in 8bb2fb74aaac861b7454ed4710d6b6d2eb0ca0c4 in #90. cc @willingc.
todo[bot] commented 5 years ago

bookstore settings review https://github.com/nteract/bookstore/pull/75/files?file-filters%5B%5D=.py#r281830978

https://github.com/nteract/bookstore/blob/671251822865f455c54b1ad9fec21ef7494198eb/bookstore/clone.py#L55-L58

This comment was generated by todo based on a TODO comment in 671251822865f455c54b1ad9fec21ef7494198eb in #90. cc @willingc.
todo[bot] commented 5 years ago

bookstore settings review https://github.com/nteract/bookstore/pull/75/files?file-filters%5B%5D=.py#r281830978

https://github.com/nteract/bookstore/blob/d1d5c2b14fc95713fe7ce6420eccff9cc4a34d56/bookstore/clone.py#L55-L58

This comment was generated by todo based on a TODO comment in d1d5c2b14fc95713fe7ce6420eccff9cc4a34d56 in #90. cc @willingc.
willingc commented 5 years ago

@mpacer I spent some time this morning going through this PR in detail. All tests are passing (thanks to M's fix to the js ci - changing const to let). Let's merge this and move forward.

We're getting very close to a release. Just a few more tests and a changelog after this lands.

willingc commented 5 years ago

@mpacer I think that this has all the changes that we discussed on our call.