-
-
Notifications
You must be signed in to change notification settings - Fork 55
Containers.all() + unit test #162
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
Conversation
@wem Here is the PR with the changes, WDYT? Actually, this is not the most correct solution (from an object-oriented perspective). We should in fact have a new implementation of the Containers interface (we would then have 2 implementations: |
Pull Request Test Coverage Report for Build 302
💛 - Coveralls |
Pull Request Test Coverage Report for Build 304
💛 - Coveralls |
Job #162 is now in scope, role is |
This pull request #162 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 |
import java.util.Iterator; | ||
|
||
/** | ||
* Containers API. |
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 we should then add a note here saying that this Iterable by default returns only the running containers
Container create(final JsonObject container) throws IOException; | ||
|
||
/** | ||
* Return all the Containers, not only the running ones (simply iterating |
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 I think it would be better if we add the "only running containers" note at the class javadoc and remove the text in these parenthesis
Perhaps leave a puzzle then? |
@llorllale I updated the Javadocs. I don't want ot leave a puzzle now, since I'm not sure it's a good idea entirely... I'll think about it and we see later :) |
@amihaiemil ok, looks good then |
@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 1 hours, bonus for fast delivery is possible (see §36) |
The job #162 is now out of scope |
Order was finished: +20 point(s) just awarded to @llorllale/z |
Payment to |
PR for #161, added option to fetch all containers, not only the running ones.