-
Notifications
You must be signed in to change notification settings - Fork 69
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
Ensure that jinja loader is not registered at each render #115
Ensure that jinja loader is not registered at each render #115
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
I recently closed #112, which introduced some conflicts to your branch. Could you take care of that? Then I'll be glad to take a closer look. |
8216bcf
to
fcdc230
Compare
@lambdaTotoro I rebased my branch on master. :) |
Hello @fbessou. You also need to adapt the new handler "ChangePasswordAdminHandler". Regards |
The env.loader was replaced by a new ChoiceLoader wrapping the previous loader on every render. It seems that the `_loaded` attribute was here to avoid re-registering the loader but it was set to `True` on the handler instance and not on its class making it useless since the a handler instance is created for each request. Closes jupyterhub#114.
fcdc230
to
aab9507
Compare
@djangoliv I adapted, my PR to take into account your modification, thanks for your review 🙂 |
I have been using your correction in production for a few days. It fix the problem. thanks a lot. |
This looks good from my end, too, I'll merge. |
The
env.loader
was replaced by a newChoiceLoader
wrapping the previousloader on every render. It seems that the
_loaded
attribute was hereto avoid re-registering the loader but it was set to
True
on thehandler instance and not on its class making it useless since the a
handler instance is created for each request.
Closes #114.