-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: adds docstrings (refactor: rename local parameter) #242
Conversation
78203d8
to
5e9c1a1
Compare
5e9c1a1
to
76530c8
Compare
nbgitpuller/__init__.py
Outdated
@@ -26,7 +46,22 @@ def load_jupyter_server_extension(nbapp): | |||
{'path': os.path.join(os.path.dirname(__file__), 'static')} | |||
) | |||
] | |||
web_app.settings['nbapp'] = nbapp | |||
web_app.settings['app'] = app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the change of importance. Will something rely on the nbapp
setting besides the one reference we have when we call it in handlers.py like this?
gp = GitPuller(repo, repo_dir, branch=branch, depth=depth, parent=self.settings['app'])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so this changes web_app.settings globally for all installed plugins, which I think is a mistake originally. I think we should just not touch web_app.settings at all, and use the initialize
method in the tornado handler instead. The third item in the tuple passed to add_handlers
can contain arbitrary dict that is passed as kwargs to an initialize
method if found. Let's use that and stop putting things in the global settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I went for adding a # FIXME note and force pushed to avoid making a reverting commit that can cause an unessecary merge conflict for Sean's plugin PR.
I opened #248 to represent this!
I think this PR is now safe to merge without any risk of complications.
Kinda of related, there's an undocumented environment variable nbgitpuller/nbgitpuller/handlers.py Line 149 in fdd54bd
Do you know it's significance? |
EDIT: Let's not discuss this here, I opened an issue #247 about it@manics it seem to have been added by me four years ago in #41 😮 I did some git history inspection:
Overall, it seems that it does the single thing of "if set to lab", the web handler for I conclude that the https://jupyterhub.github.io/nbgitpuller/link only crafts links using I'd love to see this env var and the entire |
76530c8
to
8eaf9a4
Compare
Thanks a lot for this, @consideRatio! |
This PR combines pure documentation commits with a refactoring commit that merits more closer review.
I'm not 100% it's non-breaking for all users to rename
nbapp
toapp
in the settings. To my knowledge, we only set it to be able to read it from this code base. For reference, settingnbapp
in settings was introduced in #75.