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

swagger documantation added #205

Merged
merged 1 commit into from
Jun 14, 2022
Merged

swagger documantation added #205

merged 1 commit into from
Jun 14, 2022

Conversation

sgulci
Copy link

@sgulci sgulci commented Jun 12, 2022

Hi @prabhatsharma

This PR is regarding this issue

There are some notes about this that I think they important the address these issues in the future by creating task about them

1 - The user list returns Searchresponse which is I think is wrong, but I understand that there is gonna be a UI job to change, this can be addressed by creating a new task

2 - For swagger in this first phase I did not change any object without discussing it with maintainers first, for example, it could be a good idea to return errors object every rest endpoint for a more stable API

3 - It could be a good idea to divide posts and put functions in user service, and there are not enough checks for user objects like checking Name, Password, and Role properties

4 - Same handler methods for multiple Rest endpoints is getting problem for putting them in the swagger page, we have to choose one path, little indirection could solve this problem

5 - In the bulk services there is no particular object requests, it is all done with scanner utils with reading line by line, for clarity there can be request object for this

@hengfeiyang
Copy link
Contributor

That's a great job 👍🏻

Copy link
Contributor

@hengfeiyang hengfeiyang left a comment

Choose a reason for hiding this comment

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

It's a great job. 👍🏻

@hengfeiyang hengfeiyang merged commit 42a3e38 into zincsearch:main Jun 14, 2022
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