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 js from @stormpython @captainsafia #88

Closed willingc closed 5 years ago

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

Resolve https://github.com/nteract/bookstore/pull/75#discussion_r280785802

https://github.com/nteract/bookstore/blob/402b423053a807b3c9ca476caaa38b523d95c376/ci/jupyter.js#L107-L110

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

This is good to merge if we drop the todo, as I would like to resolve the url_path_join discussion in favour of leaving it there as a convenience function.

It has saved me a lot of time and avoided a lot of pain, particularly around base_urls vs default_urls &c. with the jupyter server and it's approach to configuration.

mpacer commented 5 years ago

That said, as illustrated by #92, we aren't able to test both the GET and POST handling for cloning as of right now, only the GET.

Accordingly we didn't catch that base_urls were not being appropriately respected. I'm curious how we could accomplish this without needing to bring in the entirety of something like selenium to emulate user behaviour in this context.

@captainsafia do you have thoughts on a good way to do this without introducing too much overhead?

willingc commented 5 years ago

@mpacer Thanks and :shipit:

willingc commented 5 years ago

Usually I have push off by default on PRs. I accepted the suggestion and will merge now. Thanks @mpacer