Skip to content
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

Merged
merged 3 commits into from
Jan 18, 2022

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Jan 18, 2022

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 to app in the settings. To my knowledge, we only set it to be able to read it from this code base. For reference, setting nbapp in settings was introduced in #75.

@consideRatio consideRatio force-pushed the pr/misc-maintenance branch 7 times, most recently from 78203d8 to 5e9c1a1 Compare January 18, 2022 05:35
@consideRatio consideRatio changed the title WIP: misc maintenance docs/refactor: adds docstrings, renames reference nbapp to app (NotebookApp or ServerApp) Jan 18, 2022
@@ -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
Copy link
Member Author

@consideRatio consideRatio Jan 18, 2022

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'])

Copy link
Contributor

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?

Copy link
Member Author

@consideRatio consideRatio Jan 18, 2022

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.

@manics
Copy link
Member

manics commented Jan 18, 2022

Kinda of related, there's an undocumented environment variable NBGITPULLER_APP

app_env = os.getenv('NBGITPULLER_APP', default='notebook')

Do you know it's significance?

@consideRatio
Copy link
Member Author

consideRatio commented Jan 18, 2022

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 /git-pull endpoint will default to prepending /lab/tree to a post git-pull redirection path. It won't affect gitpuller the CLI though.

I conclude that the https://jupyterhub.github.io/nbgitpuller/link only crafts links using urlpath directly though, and the NBGITPULLER_APP is a default value for a app query parameter, which only has an influence if urlpath isn't set.

I'd love to see this env var and the entire app query parameter removed to reduce complexity, but breaking existing links isn't fun. @yuvipanda do you have a suggestion with regards to something to do with the logic about having an app parameter and/or the NBGITPULLER_APP env which is the app parameters default value?

@consideRatio consideRatio changed the title docs/refactor: adds docstrings, renames reference nbapp to app (NotebookApp or ServerApp) docs: adds docstrings (refactor: rename local parameter) Jan 18, 2022
@yuvipanda yuvipanda merged commit a97022d into jupyterhub:main Jan 18, 2022
@yuvipanda
Copy link
Contributor

Thanks a lot for this, @consideRatio!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants