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

Address base_urls properly; handle path creation on the back end #92

Closed mpacer closed 5 years ago

mpacer commented 5 years ago

Currently, the clone landing page from the GET api does not properly resolve base_urls. This addresses it.

As it relates to https://github.com/nteract/bookstore/pull/75#discussion_r280785802 and the todo in https://github.com/nteract/bookstore/pull/88: This is a case where because we lac a url_path_join utility in the front-end, it is easier to rely on the one provided by the notebook package from the backend rather than doing the text parsing for ourselves.

For example, working with base_urls causes a lot of headaches.

ipdb> self.default_url
'/blah/tree'
ipdb> self.base_url
'/blah/'

@willingc given your experience from JupyterHub & it's routing & base_url paradigms is there a better way to get access to the host name & protocol than to pull it from the request?

willingc commented 5 years ago

Instead of importing url_path_join from the notebook, let's put a url_path_join into a utils.py in the bookstore root directory. You can model the initial implementation on the code in the notebook repo.

We've done the same thing in jupyterhub. This prevents having to rely on an implementation that may change in the notebook as well as reducing the cognitive load for a new developer who will wonder why the notebook import is needed.

mpacer commented 5 years ago

@willingc Awesome, I'm happy to do that! I had thought the intention suggested by @captainsafia was to rely on external libraries when possible for this functionality.

Those are both great reasons to do this! Thank you, for the suggestions. I now know what to look for going forward as reasons to pursue these decisions.

mpacer commented 5 years ago

Those suggestions look great & have been committed! Many thanks :D!

mpacer commented 5 years ago

@willingc since you and @MSeal both approved, is it still violating the no self-merge policy for me to merge this?

mpacer commented 5 years ago

Also, thank you for merging it! 💥

willingc commented 5 years ago

@mpacer Nope. If you have addressed the feedback and folks have approved, it's fine to self merge (unless there are a bunch of complex code changes). 👍