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: operation name #843

Closed
wants to merge 1 commit into from
Closed

Conversation

yordis
Copy link
Member

@yordis yordis commented Jun 26, 2024

I tried my best to figure out how to fix the breaking changes based on the assumptions made around the x-stripeOperations extension. #829.

However, I firmly believe we should leverage the operationId since it was created for this reason. Inventing the wheel here creates more problems. Such a unique extension from Stripe helps less than they believe it does.

It is even less valuable in a language like Elixir, where the most significant information is the function, not the module. Modules are merely a grouping of functions.
Repeating information in the function name is probably a much better practice than relying on the module to give so much context (granted, a balance is a given).

@yordis
Copy link
Member Author

yordis commented Jun 26, 2024

@beam-community/team @maartenvanvliet I need your strong pushback here. Otherwise, I will be responsible for breaking the change and adequately documenting the migration.

In practice, it should be a "search" and replaced with the appropriate function, which will be clearer.

@yordis yordis marked this pull request as ready for review June 26, 2024 08:06
@yordis yordis requested a review from a team as a code owner June 26, 2024 08:06
@doomspork
Copy link
Member

@yordis sorry but I'm not sure I follow this PR, the one referenced, and the change being made here. Can you help me and the others on @beam-community/team better understand the why? Should there be test coverage here if this is fixing something?

@yordis
Copy link
Member Author

yordis commented Jul 5, 2024

@doomspork have you read #829

The generator as of today, mades assumptions about the operations that weren't inline with how Stripe generate their SDKs (which I somewhat disagree with stripe on that one since they created proprietary extensions).

The existing SDK can not longer generate the endpoints based on the latest version because it creates duplicate function definitions.

This PR is just to agree on breaking change, and unblock being able to generate latest SDK version. Relying less on whatever internal stripe extensions idea they have. Which btw, I reached out multiple times to then and dropped me in the water, I was seeking understanding in terms of how they were generating their official SDKs so I can mirror it, but I got no support.

Reasons why I rather concentrate on OpenAPI SDK (which I am familiar and hopefully as a community we figure out how to offer an extremely well maintained one).

Check the PR I linked, I just decided to backtrack out of it, chasing the internal proprietary Stripe extension is heavy on me, I am trying to make a decision that empower us, and persoanlly, unblock me. I gave enough time to the topic and I am a bit burned out by it.

If you have any Stripe connection or insights that could help me here, I much appreciate it.

@yordis yordis force-pushed the breakingchange-fix-operation-name branch 8 times, most recently from ef53a47 to 6439604 Compare July 24, 2024 16:12
@yordis yordis mentioned this pull request Aug 19, 2024
2 tasks
@yordis yordis force-pushed the breakingchange-fix-operation-name branch 5 times, most recently from 3f52a9b to 6ac61c8 Compare August 22, 2024 03:19
@yordis yordis changed the title fix!: operation name fix: operation name Aug 22, 2024
@yordis yordis force-pushed the breakingchange-fix-operation-name branch from 6ac61c8 to b1a33f0 Compare August 22, 2024 03:20
@yordis
Copy link
Member Author

yordis commented Aug 22, 2024

I decided I am not gonna break change, I will add an indirection to map existing operation id to a function name; this will kick down the road breaking changes

@yordis yordis force-pushed the breakingchange-fix-operation-name branch 5 times, most recently from b9abc4a to 080b9a5 Compare August 22, 2024 03:59
@yordis yordis force-pushed the breakingchange-fix-operation-name branch from 080b9a5 to f7dfc8e Compare August 22, 2024 04:29
@yordis yordis force-pushed the breakingchange-fix-operation-name branch from f7dfc8e to efd0eb9 Compare August 22, 2024 04:39
@yordis yordis closed this Aug 22, 2024
@yordis yordis deleted the breakingchange-fix-operation-name branch August 22, 2024 04:41
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.

2 participants