Made repo_dir an absolute path based on the server_root_dir.

[albertmichaelj]

Current behavior always clones relative to the directory that the user launches the jupyter notebook server from. This is a problem if the user either specifies --notebook-dir= or has c.NotebookApp.notebook_dir set in the config file because it may clone into a directory that is out of scope of the notebook (making the redirect fail and having unexpected clones of repos littered around the filesystem).

I've used the server_root_dir key in the nbapp.webapp.settings dictionary to get the server_root_dir and then clone relative to that. This guarantees that the clone is always in scope, and it provides the predictable behavior that the notebook root directory is always the directory into which things are cloned.

I have only tested this on Mac OS, and only with Jupyter Notebooks (not Jupyter Lab). I don't see any reason why it wouldn't work on other platforms or in Lab.

This will close #70.

[ryanlovett]

Just a request from the peanut gallery: I like your description of the issue and think it'd be great if it could be condensed into a code comment above the change.

[albertmichaelj]

No problem. I added a comment. It may be a little long, so I am fine if it needs to be cut down.

[ryanlovett]

This is great, lgtm!

[choldgraf]

nice! Is there any way to add a test to make sure that this works? I'm not sure how the tests are set up, but it seems like it'd be as simple as running a test w/ notebook_dir set and make sure that it doesn't erorr

[albertmichaelj]

I think so, though the check you run would want to have the notebook_dir set as a subdirectory of the launch directory, and then you'd just want to check to make sure the repository ended up there.

[albertmichaelj]

Can this get merged if there are no other problems? If there is something else that needs to happen from my end, let me know.

[choldgraf]

@albertmichaelj sorry for not being a bit clearer, could you add a test for this to make sure that the feature works as we'd expect in the use-case you mention? We try not to merge new code in (particularly code that fixes bugs!) without a test to make sure the bug is squashed. I'm happy to work w/ you in getting the right test code in there (maybe @ryanlovett can help out too?). Let me know what you think!

[albertmichaelj]

@choldgraf @ryanlovett Unfortunately, I think that's a little outside of my wheel house. To be honest, it's a one line change, and I don't have any experience with python unit tests. As long as server_root_dir is always set by the notebook server (and every indication that I've found is that it is), I can't see how it would ever break. Sorry that I can't help with this.

[albertmichaelj]

I was looking through tests to see if I could figure out how to write one, and I realized that none of your tests actually test the part of the code that I changed, so I'm not even sure where I'd start. As far as I can see, you only test the code in pull.py (at least with what's in tests/test_gitpuller.py), so I don't know that testing my change is actually consistent with how the project testing is currently structured. I don't even know how you would start the notebook server in your tests (which is what you'd have to do, you'd have to start the notebook server with either a config file or the --notebook-dir command line option).

However, I also pretty firmly believe that this change 1) solves a real problem, 2) it's small enough that I don't see the value in explicit testing (given that you don't test everything else in your handler.py file), and 3) it's well documented in the code what the line is doing.

Can you either merge it or explain why a test is necessary for this change?

[ryanlovett]

@albertmichaelj Its just good development practice. :) I'm a bit swamped right now otherwise I'd manually test and merge. If nobody else gets to this I can try in a couple of weeks.

[choldgraf]

@albertmichaelj first off, I want to make it clear that this is a valuable addition, and we appreciate it :-)

as @ryanlovett says, it's just good coding practice to add tests for any new code. Is this a feature that you really need in nbgitpuller now? If not, would you mind if we hold off on merging for a bit until we have the bandwidth to write some tests? If you do really need this feature in there now, I suppose we could create an issue to track the need for tests.

[albertmichaelj]

@choldgraf and @ryanlovett, I didn't mean to imply that testing wasn't best practice. My point was that your current testing infrastructure (as far as I can tell) doesn't test any of your handler.py code. So, it seems a little inconsistent to require a test for a one line change bug fix to handler.py which will likely require significant development of new testing code. I agree that it is probably best to have testing code for all of your handler.py code, but that is not what is currently in the repository. I think all of your tests just use the GitPuller class directly. If I'm mistaken about the testing infrastructure, please feel free to point out where it is, and I can try to write a small test for this.

I don't need this right now, but I do hope it's included in the next release. This is a bug fix that is important for my use case (teaching low-tech students with a local installation of anaconda launched from Anaconda Navigator).

[choldgraf]

hmm - I looked through the code and I agree with you it's unclear where would be the right place to test for this. Perhaps either @yuvipanda or @consideRatio have thoughts?

Otherwise, I'm going to open an issue that we need to add tests for the handlers.py section and we can merge this in a few days if there's not another resolution

[albertmichaelj]

Sounds good. Thanks!

[yuvipanda]

Very much appreciate the PR, @albertmichaelj!

I am hoping we can maybe get #42 in (finally!) and then a release.

We should also probably try make a concerted effort to get more maintainers for this package. I think the ratio of people who use it to maintainers could be much better. I think I've tempted @nickurak a few times now... :)

[albertmichaelj]

@yuvipanda Thanks!

[choldgraf]

woot - thanks again for the patch @albertmichaelj !