Skip to content

Conversation

amihaiemil
Copy link
Owner

PR for #161, added option to fetch all containers, not only the running ones.

@amihaiemil
Copy link
Owner Author

amihaiemil commented Oct 23, 2018

@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: RunningContainers and AllContainers, both extending RtContainers and providing their custom iterators)... but let's leave it like this for now, maybe we'll change it later.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 302

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 82.051%

Totals Coverage Status
Change from base Build 300: 0.2%
Covered Lines: 416
Relevant Lines: 507

💛 - Coveralls

@coveralls
Copy link

coveralls commented Oct 23, 2018

Pull Request Test Coverage Report for Build 304

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 82.051%

Totals Coverage Status
Change from base Build 300: 0.2%
Covered Lines: 416
Relevant Lines: 507

💛 - Coveralls

@0crat
Copy link
Collaborator

0crat commented Oct 23, 2018

Job #162 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Oct 23, 2018

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.
Copy link
Contributor

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
Copy link
Contributor

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

@llorllale
Copy link
Contributor

@amihaiemil

We should in fact have a new implementation of the Containers interface (we would then have 2 implementations: RunningContainers and AllContainers, both extending RtContainers and providing their custom iterators)

Perhaps leave a puzzle then?

@amihaiemil
Copy link
Owner Author

@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 :)

@llorllale
Copy link
Contributor

@amihaiemil ok, looks good then

@amihaiemil
Copy link
Owner Author

@rultor merge it

@rultor
Copy link
Collaborator

rultor commented Oct 23, 2018

@rultor merge it

@amihaiemil OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit e76b998 into master Oct 23, 2018
@rultor
Copy link
Collaborator

rultor commented Oct 23, 2018

@rultor merge it

@amihaiemil Done! FYI, the full log is here (took me 3min)

@amihaiemil amihaiemil deleted the 161 branch October 23, 2018 13:56
@0crat
Copy link
Collaborator

0crat commented Oct 23, 2018

Job was finished in 1 hours, bonus for fast delivery is possible (see §36)

@0crat
Copy link
Collaborator

0crat commented Oct 23, 2018

The job #162 is now out of scope

@0crat
Copy link
Collaborator

0crat commented Oct 23, 2018

Order was finished: +20 point(s) just awarded to @llorllale/z

@0crat
Copy link
Collaborator

0crat commented Oct 23, 2018

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @amihaiemil/z

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.

6 participants