Skip to content

Conversation

@srgg
Copy link
Contributor

@srgg srgg commented Nov 3, 2016

Please ensure the following is present in your PR:

  • Unit tests for your change
  • Integration tests (if applicable)

Pull requests that are small and limited in scope are most welcome.

@srgg srgg force-pushed the simplification-streamable-commands branch from 18ea4cb to c8d6ece Compare November 3, 2016 05:24
@srgg srgg force-pushed the simplification-streamable-commands branch from f1406c3 to 9630a11 Compare November 7, 2016 14:50
return future;
}

@SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of these changes look good (and needed).
The only thing that stands out are these two unchecked suppressions. We're ignoring the type system here, which always feels bad for some reason. Could we make these abstract methods, and then provide subclasses where I == CoreI and/or R == CoreR, and use those where necessary?

Sort of like what I did with the ImmediateCoreFutureAdapter: https://github.com/basho/riak-java-client/pull/677/files#diff-1c6f3b6c65d8e0eeaee247ff938a349bR70 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hate usage of whatever suppression, therefore it took some time from me to make a decision. The idea was to avoid the necessity of creating additional object such as ImmediateCoreFutureAdapter.SameQueryInfo and use Method References whenever it is possible. And also to reduce complexity I've added these default implementation that covers default scenario that doesn't require response/info conversion.

But later I realized that it might have sense to introduce another class to handle this specific case and GenericRiakComand should extend it.

@srgg srgg force-pushed the simplification-streamable-commands branch from 7b072ca to 52795d4 Compare November 8, 2016 06:39
@srgg srgg force-pushed the simplification-streamable-commands branch from 42029ea to d97183a Compare November 8, 2016 10:08
@srgg srgg added ready and removed in progress labels Nov 10, 2016
@alexmoore
Copy link
Contributor

+1

@alexmoore alexmoore merged commit a07833f into streaming-api-2 Nov 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants