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

Updating API Generation #101

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Updating API Generation #101

wants to merge 14 commits into from

Conversation

mettke
Copy link

@mettke mettke commented Oct 24, 2020

PR for #99

This is the first step for migrating to the openapi spec at https://api.slack.com/specs/openapi/v2/slack_web.json. From this brief first look, I guess that this will be a breaking change. But we will see.

ToDo:

  • Parse openapi spec
  • Generate modules
  • Generate types
  • Generate sync methods
  • Generate async methods
  • Test new API

Notes:

  • I tried to use https://crates.io/crates/openapi, however, it is not able to parse the spec from slack. Guess they do not completely adhere to the definition... The types I use were taken from openapi and were modified till they worked...
  • I have the problem I'm creating the same types multiple times, because the generator cannot tell that they are the same. Currently I'm solving this using numbers xyz, xyz1, xyz2... Not sure whether there is a better way.
  • So far it looks like error messages are sometimes missing. And the error message descriptions are always missing...

@mettke mettke marked this pull request as ready for review November 10, 2020 08:48
@mettke
Copy link
Author

mettke commented Nov 10, 2020

@brownjohnf @dvaerum @dten
After quite some efford this is ready for testing. As you @dten said, the openid slack spec is not perfect and requires some corrections, still I'm (so far) quite happy about it.

Any chance you folks have them time to help me testing? Also @dten feel free to have a look to check whether this goes into the right direction.

@mettke mettke changed the title WIP: Updating API Generation Updating API Generation Nov 10, 2020
@dten
Copy link
Contributor

dten commented Nov 10, 2020

Hey thanks for this gargantuan effort. It's gonna take me a few days to get some time to go through it. 😯😯😯

@rbtcollins
Copy link

Three thoughts -

One, merging in scraped results is probably still needed as the OpenAPI specs are super stale.

Two, the 'correct_xx' methods that are empty seem like pure noise: given the use of a wildcard match anyway, I think it would be a much smaller codebase to maintain if only the exceptions were present. e.g. in codegen/src/adapt/admin/apps/mod.rs have fn correct_approve but not correct_restrict. This places no future restriction on such fixups existing in future, but means we don't have a manual set today.

Three, rather than pulling the openapi spec from the web, if we use a git submodule, we could have a fork where these fixes are applied as a patch to the JSON schema directly, rather than in code to the parsed data structure. This might make it easier to maintain and drop them as things get fixed.

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