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

fix: codegen breaking changes in api spec #829

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yordis
Copy link
Member

@yordis yordis commented Feb 5, 2024

The existing pipeline hasn't worked since v755. I am trying to upgrade it to https://github.com/stripe/openapi/releases/tag/v756

There are some breaking changes in it, but I am not sure!

@yordis
Copy link
Member Author

yordis commented Feb 5, 2024

@maartenvanvliet, any thoughts here?

@yordis
Copy link
Member Author

yordis commented Feb 5, 2024

They are two identical update operations now without taking into consideration the path value. One for the external account, and the other one for the customer

image

@yordis
Copy link
Member Author

yordis commented Feb 5, 2024

I found the issue:

|> Enum.uniq_by(& &1["method_name"])

That is making assumptions that no other method_name should exists, but they will, the situation is problematic, because we need to lookup the operationId by the path value in order to know the final path that should be use here

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@maartenvanvliet
Copy link
Member

The https://github.com/beam-community/stripity-stripe/pull/786/files#diff-96e6fda24e580cda6be0289146011cf77e2bf6d46f3afb82dc7c628d03827ff4R2 branch has a way to override the function names for these collisions. If you find a better way I'm all for it.

@yordis
Copy link
Member Author

yordis commented Feb 5, 2024

I will bring it up to the Stripe repo to seek guidelines.

If you find a better way I'm all for it.

I am a bit pragmatic regarding this topic: just one module with all API Operations as if it was a gRPC service, as a direct example of the alternative. So I am not sure if I would add much value here!

I found what the GO SDK does: https://github.com/stripe/stripe-go/blob/9c48c72a38d0c6e066ea7bb49a93ae92b1c06143/bankaccount/client.go#L95-L102

@yordis
Copy link
Member Author

yordis commented Feb 7, 2024

@maartenvanvliet any thoughts?

@maartenvanvliet
Copy link
Member

The way the Go SDK does it, will require quite some work in the generated code. Comes down to the same thing in other SDK's by making it a special case. Personally I'd lean to a different function name.

@yordis
Copy link
Member Author

yordis commented Feb 7, 2024

Personally I'd lean to a different function name.

I agree here; I really do not like how they are doing things at all! It comes down to how could we tackle the situation, in the worst case, static list fixing the naming situation 🤷🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants