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

Javanica: basic Observable Collapser support #1320

Merged
merged 4 commits into from
Sep 7, 2016
Merged

Javanica: basic Observable Collapser support #1320

merged 4 commits into from
Sep 7, 2016

Conversation

zsoltm
Copy link
Contributor

@zsoltm zsoltm commented Aug 12, 2016

No description provided.

@mattrjacobs
Copy link
Contributor

Thanks for the contribution @zsoltm ! Are you able to include any unit tests in this PR that demonstrate how to use the Observable-Collapsers in Javanica?

Class<?> collapserReturnType = collapserMethod.getReturnType();
boolean observable = collapserReturnType.equals(Observable.class);

if (!batchReturnType.equals(List.class))
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this check is redundant because
getDeclaredMethod(obj.getClass(), hystrixCollapser.batchMethod(), List.class) already checks return type, if batch method return type isn't List then getDeclaredMethod returns null.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's legacy code. anyway it's better to remove it

@dmgcodevil
Copy link
Contributor

@zsoltm I've reviewed PR and left some comments. Also could you please update README with few examples that demonstrate new functionality. Thanks.

@zsoltm
Copy link
Contributor Author

zsoltm commented Sep 2, 2016

@dmgcodevil thanks for the review, I made the changes in a hope that this is something useful. I just needed this in my project, hacked it together, and created a PR. I'm not 100% happy the way it ended up, so if you're working on a more elegant solution, feel free to drop this.

@dmgcodevil
Copy link
Contributor

@zsoltm PR looks good from my point of view. We should try to reuse existing abstractions rather than adding new once where it's possible.

@mattrjacobs
Copy link
Contributor

Thanks for the contribution @zsoltm !

Also, thanks @dmgcodevil for the design review

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.

3 participants