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

docs: correct upload create docs (add required fields) #439

Closed
wants to merge 3 commits into from

Conversation

vermaxik
Copy link
Contributor

It's not possible to use Upload Create endpoint without those fields, they are required:

"file",
"file_format",
"locale_id"

@jablan
Copy link
Collaborator

jablan commented Nov 1, 2023

@vermaxik first, sorry for the delay, I was on vacation for a week.

The problem with this PR is that it's causing breaking change for all clients. Declaring an existing parameter as required changes method signatures. ruby example:

-    def upload_create(project_id, file, file_format, locale_id, opts = {})
-      data, _status_code, _headers = upload_create_with_http_info(project_id, file, file_format, locale_id, opts)
+    def upload_create(project_id, opts = {})
+      data, _status_code, _headers = upload_create_with_http_info(project_id, opts)
       data
     end

We'll have to discuss what to do in this case. IMO this should be merged, as it correctly describes the API, but it would need a major version bump for all the clients. Perhaps we should dedicate some time and investigate other potential places where this is needed, so that we have only one painful band-aid pull, rather than multiple ones in the future.

@theSoenke what do you think?

@theSoenke
Copy link
Collaborator

@jablan agree that we should spend some effort to check whether there are more such places to tackle all in one go. Although that might be some quite tedious work. Maybe we should then create a task for it?

@docstun
Copy link
Member

docstun commented Dec 11, 2023

@vermaxik @jablan what is the way forward here? Since tests are currently failing, I marked it as "draft" again.

@docstun docstun marked this pull request as draft December 11, 2023 08:00
@jablan
Copy link
Collaborator

jablan commented Dec 11, 2023

@docstun we opened a ticket for it https://phrase.atlassian.net/browse/TSI-2145 unfortunately, we can't make it during these quality weeks.

@theSoenke
Copy link
Collaborator

@vermaxik the logic for required fields is a bit more complex. file is definitely required but file_format and locale_id can be autodetected in some cases and are not required for all formats

@vermaxik
Copy link
Contributor Author

Hi @theSoenke

thank you for checking it, good to know.

FYI there is possibility to add discriminator/polymorphism for dynamic request/response, TMS has it a lot.

Closing this PR in favor of #571

@vermaxik vermaxik closed this Apr 22, 2024
@jablan
Copy link
Collaborator

jablan commented Apr 22, 2024

FYI there is possibility to add discriminator/polymorphism for dynamic request/response, TMS has it a lot.

@vermaxik do they also generate a set of clients from OpenAPI description? I'm a bit concerned how different clients handle such cases.

@vermaxik
Copy link
Contributor Author

@jablan looks like it should be supported https://openapi-generator.tech/docs/generators/openapi/#schema-support-feature

afaik TMS doesn't have any official clients (and they still on openapi v2), but at our team we need to implement custom support for this feature and would love to avoid it as much as we can.

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.

4 participants