-
Notifications
You must be signed in to change notification settings - Fork 42
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
Always load all protocols #278
Conversation
@@ -354,7 +354,7 @@ def import_module(module): | |||
self.mockImportModule.side_effect = import_module | |||
self.mockFindFiles.return_value = [self.FILE1] | |||
|
|||
self.makeAssignment() | |||
self.assertRaises(ImportError, self.makeAssignment) |
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.
The rationale here is that previously an invalid protocol was a config error. Now it's a bug.
Does this mean that we won't need to add |
Yes to both. Is there a danger to letting students run |
I don't think so. I was just clarifying my own understanding. |
The only protocol I'm concerned about is Backup. It's perfectly valid for someone not want to use the backup protocol (data8 did this all last year) and with this PR there is no way to do so (without having the user add arguments every time). No other protocol prompts for authentication with a user flag specifying that they want to run that protocol. There are a few solutions:
|
c356ae1
to
b937ea1
Compare
Hmm how about omitting the endpoint entirely? |
We use the endpoint all over the place - in collab & hinting for example. If we can get a default value in there - so that using |
The endpoint now defaults to the empty string if omitted. If the endpoint is falsey, we won't attempt to backup. I tested the other protocols too manually, and nothing seemed to break. |
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.
I think this is worth a bump to v1.10.0
From now on we'll ignore the list of protocols in
protocols
.