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

Added rooms/:roomId/users query parameters to client API #12

Closed
wants to merge 3 commits into from
Closed

Added rooms/:roomId/users query parameters to client API #12

wants to merge 3 commits into from

Conversation

asantibanez
Copy link

Only for Rx version.

  1. Added query parameters to client api.
  • q: Search query
  • skip: Skip n users.
  • limit: maximum number of users to return (default 30).
  1. Added test dependencies
  • JUnit
  • Hamcrest
  1. Added tests for new api. Tests available in rx/src/test/java

… for new client methods. Only for Rx version
@asantibanez
Copy link
Author

This is my first PR. I think your SDK is awesome. Would like to enhance some endpoints to complete Gitter API.

@amatkivskiy
Copy link
Member

Hi @asantibanez. Thanks for help 👍 Additionally great thanks for adding testing to this library 👍 👍
I have couple of notes regarding the changes:

  • Have you tested that Gitter API with skip and limit params? Cause in my case this params had no influence on the response items.
  • Maybe you can change the architecture of skip,query and limit to something like it was implemented for ChatMessagesRequestParams?
  • Regarding testing architecture: in such cases (like this library) the best way to implement integration testing is through mocking webserver calls. This makes tests more stable, quicker and there will be no need to have a private token in the tests. I've implemented such testing for getRoomUsers(roomId) request. Please look on the this PR.

@asantibanez
Copy link
Author

Thanks @amatkivskiy! I will try to implement the skip, limit, and query params like the architecture you have for ChatMessagesRequestParams.

Also, will look into testing locally as you suggest. I understand why local testing of the web server is better. I didn't know how to do it. Thanks for pointing out the direction.

@amatkivskiy
Copy link
Member

@asantibanez May I close this request?

@asantibanez
Copy link
Author

Yup. Go ahead. Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants