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

(#996) Make ListEnvelope public and consolidate API #1096

Merged
merged 1 commit into from
Apr 21, 2019

Conversation

victornoel
Copy link
Collaborator

This is for #996:

  • made ListEnvelope public and clarified doc on its behaviour
  • Ensured subList was returning a List with the same behaviour (note that it means sublist is now called lazily)
  • Added some tests for this
  • Prevented overriding of toString and simplified Mapped as a result

@0crat 0crat added the scope label Apr 6, 2019
@0crat
Copy link
Collaborator

0crat commented Apr 6, 2019

Job #1096 is now in scope, role is REV

@codecov-io
Copy link

codecov-io commented Apr 6, 2019

Codecov Report

Merging #1096 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1096   +/-   ##
=========================================
  Coverage     88.77%   88.77%           
+ Complexity     1588     1587    -1     
=========================================
  Files           275      275           
  Lines          3947     3947           
  Branches        214      214           
=========================================
  Hits           3504     3504           
  Misses          390      390           
  Partials         53       53
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/cactoos/list/ListIteratorOf.java 94.44% <ø> (ø) 12 <0> (ø) ⬇️
src/main/java/org/cactoos/list/Mapped.java 100% <ø> (ø) 1 <0> (-1) ⬇️
...ava/org/cactoos/collection/CollectionEnvelope.java 100% <ø> (ø) 28 <0> (ø) ⬇️
src/main/java/org/cactoos/list/ListEnvelope.java 100% <100%> (ø) 12 <2> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00e5f23...f71ae47. Read the comment docs.

@0crat
Copy link
Collaborator

0crat commented Apr 14, 2019

This pull request #1096 is assigned to @vzurauskas/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/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

Copy link
Contributor

@vzurauskas vzurauskas left a comment

Choose a reason for hiding this comment

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

@victornoel Review done.

}

@Override
public String toString() {
public final String toString() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider removing this toString entirely. I think it should be final in parent abstract class, they both decorate the same List object anyway.

}

@Test(expected = UnsupportedOperationException.class)
public void unsupportedRemove() {
Copy link
Contributor

Choose a reason for hiding this comment

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

removeIsNotSupported, to be consistent with naming of other tests?

}

@Test(expected = UnsupportedOperationException.class)
public void unsupportedSet() {
Copy link
Contributor

Choose a reason for hiding this comment

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

setIsNotSupported, to be consistent with naming of other tests?

}

@Test(expected = UnsupportedOperationException.class)
public void unsupportedAdd() {
Copy link
Contributor

Choose a reason for hiding this comment

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

addIsNotSupported, to be consistent with naming of other tests?

@victornoel
Copy link
Collaborator Author

@vzurauskas thanks, I've applied your recommendations and also added a @todo so that toString() can be made final in CollectionEnvelope (it's not so obvious so I didn't do it).

Copy link
Contributor

@vzurauskas vzurauskas left a comment

Choose a reason for hiding this comment

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

@llorllale Looks good.

@piotrkot
Copy link

Guys, when you merge it, can you make a new release with this fix?

@llorllale
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Apr 21, 2019

@rultor merge

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

@rultor rultor merged commit f71ae47 into yegor256:master Apr 21, 2019
@rultor
Copy link
Collaborator

rultor commented Apr 21, 2019

@rultor merge

@llorllale Done! FYI, the full log is here (took me 12min)

@0crat 0crat removed the scope label Apr 21, 2019
@0crat
Copy link
Collaborator

0crat commented Apr 21, 2019

@sereshqua/z please review this job completed by @vzurauskas/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

@0crat
Copy link
Collaborator

0crat commented Apr 21, 2019

The job #1096 is now out of scope

@sereshqua
Copy link

sereshqua commented Apr 21, 2019

@piotrkot please make sure, that next and all other messages you write, will start with name / names of a user / users they are addressed to as written in policy

@llorllale
Copy link
Contributor

@rultor release, tag is 0.41

@rultor
Copy link
Collaborator

rultor commented Apr 21, 2019

@rultor release, tag is 0.41

@llorllale OK, I will release it now. Please check the progress here

@0crat
Copy link
Collaborator

0crat commented Apr 21, 2019

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

@rultor
Copy link
Collaborator

rultor commented Apr 21, 2019

@rultor release, tag is 0.41

@llorllale Done! FYI, the full log is here (took me 14min)

@sereshqua
Copy link

@piotrkot please see my comment

@piotrkot
Copy link

@sereshqua I saw your comment when it appeared. No need to comment about past comment ;)

@sereshqua
Copy link

@piotrkot you saw, but you haven't replied :)

@sereshqua
Copy link

@0crat quality acceptable

@0crat
Copy link
Collaborator

0crat commented Apr 23, 2019

Quality review completed: +4 point(s) just awarded to @sereshqua/z

@0crat
Copy link
Collaborator

0crat commented Apr 23, 2019

Order was finished, quality is "acceptable": +15 point(s) just awarded to @vzurauskas/z

@piotrkot
Copy link

@sereshqua I didn't answer as it wasn't a question ;)

@victornoel victornoel deleted the 996-listenvelope-public branch February 15, 2020 14:07
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.

9 participants