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

user_status: Add OpenAPI spec #39133

Merged
merged 1 commit into from
Jul 7, 2023
Merged

user_status: Add OpenAPI spec #39133

merged 1 commit into from
Jul 7, 2023

Conversation

provokateurin
Copy link
Member

Summary

From #36666. Adds the necessary annotations and descriptions.

Checklist

@provokateurin provokateurin added the 3. to review Waiting for reviews label Jul 4, 2023
@provokateurin provokateurin added this to the Nextcloud 28 milestone Jul 4, 2023
@provokateurin
Copy link
Member Author

@nickvergessen I agree with your points, we could try to keep 100% compatibility but then we have a pretty bad API documentation (only because of the current API of course). The changes are only done to make the API more sensible and easier to use. I'm also pretty sure this breaking change might impact nothing because clients probably ignore the content already

@nickvergessen
Copy link
Member

The changes are only done to make the API more sensible and easier to use. I'm also pretty sure this breaking change might impact nothing because clients probably ignore the content already

My point is more that this here is a sample. All kind of apps and app devs will adjust to this sample behaviour and break any of their APIs where they use [] as data (which is still the default)

@provokateurin
Copy link
Member Author

So are you arguing for or against this breakage? I'm not really sure what you are telling me. As I said I can also remove the compatibility break, but then we stay with this ugly API. Maybe this should be discussed differently, because I only want to document the current state and not improve it (but it happened to me many times that I saw a bad API and tried to fix it)

@provokateurin
Copy link
Member Author

Ok I will restore compatibility and type the value as array<empty> which is validated properly in psalm and is also useful in the spec.

@nickvergessen
Copy link
Member

So are you arguing for or against this breakage?

I'm always against breaking APIs if it is not necessary.

As I said I can also remove the compatibility break, but then we stay with this ugly API.

¯\_(ツ)_/¯

Maybe this should be discussed differently, because I only want to document the current state and not improve it (but it happened to me many times that I saw a bad API and tried to fix it)

End of July?

Ok I will restore compatibility and type the value as array<empty> which is validated properly in psalm and is also useful in the spec.

Thanks, sounds good 👍🏼

Signed-off-by: jld3103 <jld3103yt@gmail.com>
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷🏼

@blizzz blizzz merged commit 9bf6911 into master Jul 7, 2023
@blizzz blizzz deleted the feature/openapi/user_status branch July 7, 2023 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants