ok-client icon indicating copy to clipboard operation
ok-client copied to clipboard

Refactor the "protocol" abstraction

Open knrafto opened this issue 9 years ago • 4 comments

The current system reads a list of protocol names in the config file and runs them in order. This has several disadvantages:

  • The protocols must be specified in the correct order.
  • It is very easy to forget to add a protocol in the config file - generally you just want to run them all.
  • In many cases we should only run one "protocol", depending on the command line options. For example, with --unlock, we only want to run unlocking; and we don't want to run it at all if the flag is not given.

Overall I feel that this is not the right abstraction. Instead we should structure them more as "commands", similar to git commands or npm commands. For example:

  • The default command runs tests, backs up file contents, collects analytics data, and checks for updates.
  • For --unlock, run the unlocking. This includes experiments such as guidance messages.
  • For --backup, --submit, and --revise, talk to the server.
  • For --lock, lock the tests.
  • For --score, run the tests and dump the results.
  • For --update, try to update the client.
  • For --authenticate, reauthenticate.

These commands should all be mutually exclusive - it is an error to specify more than one of these at a time.

knrafto avatar Aug 12 '16 08:08 knrafto

This should also make it easier to add new commands that integrate with other services, like --collaborate or --debug-online.

knrafto avatar Aug 16 '16 20:08 knrafto

This might not be as clean as we want because the most commonly used actions are not single purpose commands - unlock and backup for example are not single purpose actions.

Multi step flags: no flags/default: Analytics, Run tests until first test fails, backup/talk to server, check for update --unlock: Analytics, Unlocking, backup, check for update --backup/submit/revise : Analytics, run all tests, backup/talk to server, check for update -q 20: Analytics, run tests for Q20, backup/talk to server, check for update

Single purpose commands: --lock: Lock --score: Score --update: Check for updates --auth: Auth

A question that comes up is:

  • How does an assignment specify that it doesn't support a specific feature?
    • Example use case: What happens if a course wants to run a standalone ok assignment without backups/talking to the server.
    • Potential work around: It's possible that there would be a config to specify the default behavior.

I agree that the protocol abstraction makes implementing flags that are available to any assignment harder and that having to order the list in particular makes it annoying to setup an assignment.

@albert12132: Any thoughts on this?

Sumukh avatar Aug 16 '16 20:08 Sumukh

@knrafto Do you think #278 closes this? It's not quite a refactor but solves most of the disadvantages (except running a single protocol)

Sumukh avatar Feb 28 '17 06:02 Sumukh

I'd like to keep this open for "subcommands" - right now adding a command requires modifying all the protocols

knrafto avatar Feb 28 '17 22:02 knrafto