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

Organize into multiple files #2

Closed
wants to merge 2 commits into from
Closed

Organize into multiple files #2

wants to merge 2 commits into from

Conversation

yuvipanda
Copy link
Collaborator

  • Allow LTILaunchValidator to be used without needing to use all
    the JupyterHub related things!
  • Use a specific LTLaunchValidationError than just web.HTTPError

- Allow LTILaunchValidator to be used without needing to use all
  the JupyterHub related things!
- Use a specific LTLaunchValidationError than just web.HTTPError
@yuvipanda yuvipanda requested a review from minrk February 13, 2018 06:43
user = yield self.login_user()
self.redirect(self.get_body_argument('custom_next', self.get_next_url()))
from .auth import LTIAuthenticator
from .validator import LTILaunchValidator, LTILaunchValidationError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you omit the from .auth import LTIAuthenticator here in __init__, instead requiring users to explicitly import litauthenticator.validator or ltiauthenticator.auth, then the validator may be used without jupyterhub being importable.

That's a backward-incompatible change in terms of imports, but a minor one.

@jgwerner
Copy link
Collaborator

@yuvipanda should we close this now that we have the refactor done with #41 ?

@jgwerner
Copy link
Collaborator

@consideRatio would you agree on closing this PR since the merge from #44 effectively updated the master branch with these changes?

@yuvipanda
Copy link
Collaborator Author

Yep, let's close this. Thanks, @jgwerner!

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

Successfully merging this pull request may close these issues.

3 participants