-
Notifications
You must be signed in to change notification settings - Fork 174
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
python: Use openapi-codegen for high-level python API client #1646
base: main
Are you sure you want to change the base?
Conversation
0a45271
to
f724642
Compare
(I force-pushed a typo fix in the commit message) |
f342fb5
to
6cb2f29
Compare
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.
Looks pretty good! I think we just need to document the breaking changes, then we can merge.
self, | ||
app_id: str, | ||
message_in: MessageIn, | ||
options: MessageCreateOptions = MessageCreateOptions(), |
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.
@svix-jplatte, I wonder if we want to break this? I guess it's fine and we can just swallow this? Though if we do, just need to make sure it's the last Python break.
I will go through the diffs one at a time and document the breaking changes for the |
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 have lots of thoughts on these changelog entries 😅
ChangeLog.md
Outdated
* Libs/Python **(Breaking)**: `PostOptions` and `ListOptions` are no longer used in methods for `Authentication`,`Endpoint`,`EventType`,`Integration`,`MessageAttempt`,`Message` and `Statistics` resources. Instead each API call now has it's own ResourceMethodsOptions. (Both sync and async) | ||
* Libs/Python: `Application.dashboard_access` is not marked as deprecated. (Both sync and async) | ||
* Libs/Python **(Breaking)**: `EndpointStatsOptions` is renamed to `EndpointGetStatsOptions` | ||
* Libs/Python **(Breaking)**: For `Endpoint.patch_headers` the `endpoint_headers_in` argument is rename to `endpoint_patch_headers_in` (Both sync and async) |
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.
typo: "is rename"
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.
This is a breaking change because every Python argument can be passed as name=value
by default? I wonder if it's worth noting down..
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'm seeing a bunch more like this below. I think they're decreasing the signal-noise ratio a bit. If you need sbd. else to make a decision ask Tom but I'd be inclined to skip these, seems extremely unlikely that people are using named arguments for these methods with very few parameters, and should be very easy to adjust to w/o reading the changelog as well.
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.
TBH I wouldn't be surprised if people are, but I'm happy with making it more a blanket statement.
We just need to make sure to clarify that this is a one-time break, and that everything should be easy to adjust manually.
Note: the openapi.json file I am working with does not include some deprecated methods. For now I will manually add these deprecated calls by hand
Breaking change: in the create and update function the integ_(in/update) is renamed to integration_(in/update) This is due to the way that the codegen decides how to name function arg. (The type of the function arg to_snake_case) I don't think it makes sense to add a special case in the codegen to shorten `integration` to `integ` This problem is a bit more annoying, since the path param is called `integ_id` (in the `openapi.json`). So to keep consistency we may want to add the special case to the codegen Alternatively (and this would make me happy) we could change the path param name from `integ_id` to `integration_id`. (in the `openapi.json`) This will make the file consistent, and won't require a special case in the openapi-codegen
Braking changes: - `MessageAttemptListOptions` is not importable from `svix.api`
Manual edits: - For `v1.message.create` manually set `ret.payload` to `message_in.payload` - For `MessageCreateOptions` set the `with_content` false by default. (this is preserve previous behavior)
This also switches the `MessageListOptions` import from `message_attempt` to `message`, and removes the definition in `message_attempt` since it is not used by anything (except the `from svix.api` import, which we replaced) Breaking change: - The `status_code_class` and `status` optional fields are removed from `svix.api.MessageListOptions`
Technically? breaking change: - the `task_id` argument is removed from the `aggregate_event_types` method. However this argument was not previously used by the inner API client at all. So this technically breaks the high-level API client, but no change to the actual outgoing api.svix.com requests
a2949fd
to
0406384
Compare
The
openapi.json
file I am working with is incomplete.openapi.json
.background_task
andoperational_webhook
. (I know thatoperational_webhook
is not a single resource)openapi.json
are not currently in the high-level API client (health
andenvironment
).Jonas is working on a fix for the
openapi.json
generator.For now.
@deprecated
decorator to those methods.openapi.json
, I will leave it as is.openapi.json
, I will ignore them for now.Part of https://github.com/svix/monorepo-private/issues/9675