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

Implemented a mock http client for Issue #459 #483

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jthomasprogrammer
Copy link

Put the httpclient reference behind a interface to add the ability for
deterministic unit testing of api interactions in the future.

Put the httpclient reference behind a interface to add the ability for
deterministic unit testing of api interactions in the future.
Copy link
Owner

@BenFradet BenFradet left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , I'll let @JanOuborny have a look

@BenFradet BenFradet requested a review from JanOuborny October 21, 2017 11:27
@JanOuborny
Copy link
Contributor

JanOuborny commented Oct 21, 2017

The problem here is, that there is already a pending implementation of loose coupling of the HttpClient:
https://github.com/BenFradet/RiotSharp/pull/481/files#diff-c1fcfa7f3de2007b100a52b0b10ea792R9

We need to merch both of these changes and I propose that we do that in a different branch.

@jthomasprogrammer
Copy link
Author

Gotcha, didn't realized there was already a pending implementation. Should I do anything on my end to help with the merch of the changes?

@BenFradet
Copy link
Owner

@averageprogrammer yup it'd be nice if you could have a look and review the other PR since you've done something similar 👍

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