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
Docker.httpClient() + unit tests #166
Conversation
Job #166 is now in scope, role is |
Pull Request Test Coverage Report for Build 319
💛 - Coveralls |
This pull request #166 is assigned to @llorllale/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @amihaiemil/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job |
).httpClient(), | ||
Matchers.allOf( | ||
Matchers.notNullValue(), | ||
Matchers.instanceOf(HttpClient.class), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amihaiemil this 'instanceOf' check seems unnecessary
).httpClient(), | ||
Matchers.allOf( | ||
Matchers.notNullValue(), | ||
Matchers.instanceOf(HttpClient.class), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amihaiemil same here: this 'instanceOf' check seems unnecessary
@amihaiemil left you a couple of minor comments. What really bugs me though is the premise behind this issue. What's wrong with |
@llorllale The premise is simply that the client might want to perform HTTP requests which are not yet implemented by the library. And, in case of a local Docker, the HttpClient would need to be configured to work over a unix socket (which is not a trivial task, you really have to know how Apache HttpClient works). So, since we already did that, we might as well let the user get it and use it directly :D |
@amihaiemil I think you'll have to expose the baseURI as well |
@llorllale That won't be necessary since the user will build their own HttpRequest and they should already know the base URI. Besides, our HttpClient does not contain any base URI, only the connection information (e.g. that File socket). |
@amihaiemil alright, go ahead then (good to merge) |
@rultor merge it |
@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here |
@amihaiemil Done! FYI, the full log is here (took me 3min) |
Job was finished in 30 hours, bonus for fast delivery is possible (see §36) |
The job #166 is now out of scope |
Order was finished: +20 point(s) just awarded to @llorllale/z |
Payment to |
PR for #165