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

feat: Add OpenAPI #3324

Merged
merged 1 commit into from
Nov 12, 2024
Merged

feat: Add OpenAPI #3324

merged 1 commit into from
Nov 12, 2024

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Oct 7, 2024

Closes #3208
This is a "breaking" change to the API, but since it was not usable before due to CSRF restrictions it doesn't matter.
While working on this I also had to cleanup a lot of the internal typings which resulted in this relatively large change for adding OpenAPI.
If this is too big please let me know and I will try to split it up.

@provokateurin provokateurin added enhancement 3. to review Items that need to be reviewed labels Oct 7, 2024
@provokateurin provokateurin added this to the Nextcloud 31 milestone Oct 7, 2024
@provokateurin provokateurin marked this pull request as draft October 7, 2024 10:02
README.md Outdated Show resolved Hide resolved
lib/Command/ListCommand.php Outdated Show resolved Hide resolved
lib/Command/ListCommand.php Outdated Show resolved Hide resolved
lib/Folder/Folder.php Outdated Show resolved Hide resolved
lib/Folder/FolderManager.php Outdated Show resolved Hide resolved
lib/Folder/FolderMapping.php Outdated Show resolved Hide resolved
@provokateurin
Copy link
Member Author

I will try to extract some of the changes into separate PRs to get them merged first and then come back to this PR.

@provokateurin
Copy link
Member Author

I removed everything not strictly necessary to implement this. Some improvements can just be done later, like nextcloud/openapi-extractor#172.

@provokateurin provokateurin marked this pull request as ready for review October 28, 2024 14:43
@provokateurin
Copy link
Member Author

The Cypress failure is not related to the changes, I get the same errors on master.

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Why removing all the id indexing, is that so bad?
Also, why the snake_case on string ids, do we have a convention about that in OCS?

lib/Folder/FolderManager.php Outdated Show resolved Hide resolved
@provokateurin
Copy link
Member Author

Why removing all the id indexing, is that so bad?

Not really, but I just removed it because it is never used.
I prefer a list strongly over array if possible, due to the ambiguities that can happen.
I can also revert it, if you really don't like it.

Also, why the snake_case on string ids, do we have a convention about that in OCS?

No, I just wanted to make them all consistent.
I personal prefer snake_case for JSON, but we can also go with camelCase.

@susnux
Copy link
Contributor

susnux commented Nov 4, 2024

prefer a list strongly over array if possible, due to the ambiguities that can happen.

True, I think array caused really some unwanted issues :(

I personal prefer snake_case for JSON, but we can also go with camelCase.

I think we use camelCase for both PHP and JS variables and for JS working with camelCase JSON is more convenient. So if we somewhen might consider a codestyle also JSON camelCase would be nice

@provokateurin
Copy link
Member Author

I think we use camelCase for both PHP and JS variables and for JS working with camelCase JSON is more convenient. So if we somewhen might consider a codestyle also JSON camelCase would be nice

Ok, fair enough :)

@provokateurin
Copy link
Member Author

Why removing all the id indexing, is that so bad?

Also the code on the frontend side was very hacky in this regard, it is a lot better now.

@provokateurin
Copy link
Member Author

Depends on #3407 and nextcloud/openapi-extractor#181.
No breaking changes anymore, but also very ugly typing 🙈

@provokateurin provokateurin changed the base branch from master to fix/routes/index-php November 11, 2024 13:24
Base automatically changed from fix/routes/index-php to master November 11, 2024 14:29
Signed-off-by: provokateurin <[email protected]>
@provokateurin
Copy link
Member Author

All depending PRs are merged, so this is finally ready.

@provokateurin
Copy link
Member Author

Still waiting for a review from @come-nc before merging to ensure I addressed all previous problems.

@provokateurin provokateurin merged commit 3621feb into master Nov 12, 2024
47 checks passed
@provokateurin provokateurin deleted the feat/openapi branch November 12, 2024 08:43
@provokateurin
Copy link
Member Author

Thanks a lot everyone! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Items that need to be reviewed enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OpenAPI documentation
5 participants