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

(#1254) Implements ListIteratorJoined #1548

Merged

Conversation

baudoliver7
Copy link
Contributor

@baudoliver7 baudoliver7 commented Feb 26, 2021

For #1254

  • Implements JoinedListIterator
  • Introduces unit tests for JoinedListIterator

@0crat
Copy link
Collaborator

0crat commented Feb 26, 2021

@victornoel/z everybody who has role REV is banned at #1548; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

Copy link
Collaborator

@victornoel victornoel left a comment

Choose a reason for hiding this comment

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

@baudoliver7 on top of the comment, I propose to have all the mutable operations (I think remove, set and add) throw UnssuportedOperationException and add a todo to think about a smart solution to implement them. Because for now I'm not comfortable with the proposed implementation, it seems fragile to rely on emptyListIterator like this... The todo should clearly state that the javadoc of those operations should be well respected and also that the behaviour of adding/setting/removing an element when the cursor is in between two listiterators be well documented in this class (because there are multiple options in that case, no?).
Alternatively, we keep them, use hasPrevious/hasNext instead of checking the empty list iterator and add a todo to clarify doc and ensure the implementation is correct as explained above.

src/main/java/org/cactoos/list/ListIteratorJoined.java Outdated Show resolved Hide resolved
src/main/java/org/cactoos/list/ListIteratorJoined.java Outdated Show resolved Hide resolved
src/main/java/org/cactoos/list/ListIteratorJoined.java Outdated Show resolved Hide resolved
src/main/java/org/cactoos/list/ListIteratorJoined.java Outdated Show resolved Hide resolved
src/main/java/org/cactoos/list/ListIteratorJoined.java Outdated Show resolved Hide resolved
@victornoel
Copy link
Collaborator

@baudoliver7 hi, how is it going for this PR? :)

@baudoliver7
Copy link
Contributor Author

baudoliver7 commented Mar 6, 2021 via email

@victornoel
Copy link
Collaborator

@baudoliver7 no rush, I mostly wanted to be sure it wasn't forgotten :)

@baudoliver7 baudoliver7 force-pushed the list_joined_is_uselessly_copying_data branch from 3d2fa40 to 5356174 Compare March 7, 2021 22:57
@baudoliver7
Copy link
Contributor Author

@victornoel Please, could you have a look at my new changes ?

@victornoel
Copy link
Collaborator

@baudoliver7 thx

@victornoel
Copy link
Collaborator

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Mar 8, 2021

@rultor merge

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

@rultor rultor merged commit 870678d into yegor256:master Mar 8, 2021
@rultor
Copy link
Collaborator

rultor commented Mar 8, 2021

@rultor merge

@victornoel Done! FYI, the full log is here (took me 9min)

@0crat 0crat added qa and removed 0crat/scope labels Mar 8, 2021
@0crat
Copy link
Collaborator

0crat commented Mar 8, 2021

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

@baudoliver7 baudoliver7 deleted the list_joined_is_uselessly_copying_data branch March 8, 2021 17:38
@sereshqua
Copy link

@0crat quality bad

@0crat 0crat added quality/bad and removed qa labels Mar 8, 2021
@0crat
Copy link
Collaborator

0crat commented Mar 8, 2021

Quality is low, no payment, see §31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants