Non-Generated WEBKNOSSOS API Client

[fm3]

Description:

• Replaces the generated webknossos api client by a handwritten one
• The client generation was a good idea, but did not work well in practice, in part due to incomplete swagger information by the outdated swagger-play library.
• The handwritten client is very similar to the one used in the javascript frontend
• It adds type-safety and holds information about which fields are optional.
• It contains only those routes and fields that are actually used by the client. Add what’s needed
• It also queries the versioned api routes and adds some more explanatory texts to the exceptions in failure cases.
• The PR also refreshes the test cassettes to match the requests performed by the new client (pretty similar to before, though)

Issues:

• contributes to Get rid of swagger annotations webknossos#7337

Todos:

• proof of concept
• separate datastore routes
• camelCase to snake_case for attr fields?
• merge master
• migrate all the routes
  • hurdles:
    • retries
    • X-Total-Count header for pagination (e.g. tasks)
    • weird format of task creation response
    • experiences are key:value dict, should not be auto_snake_cased, how to prevent that there?
• remove the generated client and generation code
• remove logging. version hint should go into the exception so users can catch it if they want
• adapt and extend automated tests
• remove pyhumps dependency (and others?)
• refresh test cassets
• do some manual testing
• cleanup
• CI
• Document how to modify this in the future
  --
• Updated Changelog
• Added / Updated Tests

[philippotto]

Great stuff 👍 I like that the glue code got much more readable due to the better typing (which is obviously awesome in itself).

I left some small comments. Other than that, I stumbled a bit about the naming of the methods in the client. Simply retrieving something is named à la folder_tree() (without "get" or "fetch") and updating something is dataset_team_update (instead of update_team_for_dataset). You probably tried to keep the names as close as possible to the routes and only added the verbs when the method is not GET? I think, it's fine, since this layer is only a thin abstraction used by the actual model classes. Still wanted to write my thoughts down for the record..

[fm3]

Thanks for taking the time to read through this!

I see your point concerning the api method naming. Most names are close to what the generated client had. However, the names may not be ideal, and currently they are kind of mixed.

I think there are two possible ways to design the names:

• (1) use verbs first (update_dataset_teams, create_tasks_from_files) (I think I’d still skip the get, though)
• (2) use domain first (dataset_update_teams, dataset_assert_new_name_is_valid, annotation_upload, user_logged_time). This has the benefit of automated grouping by domain, which could help with auto-completion in the IDE

Most routes currently use approach 2. Depending on your opinion I could now either iron the rest out by adapting this domain first approach (originally introduced by jonathan) for the remaining few methods, or I could use the verb-first approach (with or without "get"). What do you think?

[philippotto]

I think there are two possible ways to design the names:

• (1) use verbs first (update_dataset_teams, create_tasks_from_files) (I think I’d still skip the get, though)
• (2) use domain first (dataset_update_teams, dataset_assert_new_name_is_valid, annotation_upload, user_logged_time). This has the benefit of automated grouping by domain, which could help with auto-completion in the IDE

Most routes currently use approach 2. Depending on your opinion I could now either iron the rest out by adapting this domain first approach (originally introduced by jonathan) for the remaining few methods, or I could use the verb-first approach (with or without "get"). What do you think?

I'm fine with either way as long as it is consistent (also I'd add a comment to the module which explains the naming). So, feel free to go for (2).

[philippotto]

🎉

[jstriebel]