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

#1549: Add ctor to map.Merged #1550

Merged
merged 5 commits into from Mar 3, 2021
Merged

#1549: Add ctor to map.Merged #1550

merged 5 commits into from Mar 3, 2021

Conversation

andreoss
Copy link
Contributor

Per #1549

  • Add extra ctor

@andreoss andreoss changed the title (#1549) Add extra ctor #1549: Add ctor to map.Merged Feb 27, 2021
@andreoss andreoss marked this pull request as ready for review February 27, 2021 23:30
Comment on lines 44 to 51
new Merged<>(
new MapOf<>(
new MapEntry<>("a", 1)
),
new MapOf<>(
new MapEntry<>("b", 2)
)
),
Copy link
Contributor

Choose a reason for hiding this comment

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

@andreoss I think you wanted to test Merged with Iterable constructor :) It should be like this :

new Merged<>(
    new IterableOf<>(
          new MapOf<>(
              new MapEntry<>("a", 1)
          ),
          new MapOf<>(
              new MapEntry<>("b", 2)
          )
    )
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baudoliver7 Actually not, there's not need to test Iterable ctor because it is primary and getting hit anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

@andreoss That's right ! But it's because of the test name (behavesAsMapCreatedFromIterable) that I said that. However, you change it in your last commit.

@Test
public void behavesAsMapCreatedFromIterable() {
new Assertion<>(
"Must behave as a map",
Copy link
Contributor

Choose a reason for hiding this comment

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

@andreoss I suggest this message : Must behave as a map when created from iterable


/**
* Ctor.
* @param maps Maps to merge.
Copy link
Contributor

Choose a reason for hiding this comment

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

@andreoss I suggest to write as description : Iterable of {@link Map}s to merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baudoliver7 Fixed.

Copy link
Contributor

@baudoliver7 baudoliver7 left a comment

Choose a reason for hiding this comment

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

@andreoss Please, see my comments.

@codecov-io
Copy link

codecov-io commented Feb 28, 2021

Codecov Report

Merging #1550 (963a5ba) into master (ffe6721) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1550      +/-   ##
============================================
- Coverage     90.85%   90.82%   -0.03%     
- Complexity     1567     1570       +3     
============================================
  Files           291      291              
  Lines          3694     3693       -1     
  Branches        121      121              
============================================
- Hits           3356     3354       -2     
  Misses          308      308              
- Partials         30       31       +1     
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/cactoos/map/Merged.java 100.00% <100.00%> (ø) 2.00 <1.00> (ø)
src/main/java/org/cactoos/scalar/Solid.java 90.00% <0.00%> (-10.00%) 3.00% <0.00%> (-1.00%)

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 ffe6721...963a5ba. Read the comment docs.

Copy link
Contributor

@baudoliver7 baudoliver7 left a comment

Choose a reason for hiding this comment

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

@andreoss Thanks. @victornoel It looks good to me. Please, check/merge

@victornoel
Copy link
Collaborator

@andreoss @baudoliver7 thx

By the way @andreoss, no need to create a ticket if you plan to create a PR directly :)

@victornoel
Copy link
Collaborator

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Mar 3, 2021

@rultor merge

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

@rultor rultor merged commit 963a5ba into yegor256:master Mar 3, 2021
@rultor
Copy link
Collaborator

rultor commented Mar 3, 2021

@rultor merge

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

@0crat 0crat added the qa label Mar 3, 2021
@0crat
Copy link
Collaborator

0crat commented Mar 3, 2021

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

@0crat 0crat removed the 0crat/scope label Mar 3, 2021
@sereshqua
Copy link

@0crat quality good

@0crat 0crat added quality/good and removed qa labels Mar 3, 2021
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.

None yet

7 participants