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

(#1082) Replace forbidden API calls #1114

Merged
merged 1 commit into from
Apr 25, 2019
Merged

(#1082) Replace forbidden API calls #1114

merged 1 commit into from
Apr 25, 2019

Conversation

vladhss
Copy link
Contributor

@vladhss vladhss commented Apr 23, 2019

This is for #1082
I replaced forbidden API calls in org.cactoos.collection package, updated to-do.

@0crat 0crat added the scope label Apr 23, 2019
@codecov-io
Copy link

codecov-io commented Apr 23, 2019

Codecov Report

Merging #1114 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1114   +/-   ##
=========================================
  Coverage     88.77%   88.77%           
  Complexity     1588     1588           
=========================================
  Files           275      275           
  Lines          3946     3946           
  Branches        214      214           
=========================================
  Hits           3503     3503           
  Misses          390      390           
  Partials         53       53

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 b74f80c...622165a. Read the comment docs.

@0crat
Copy link
Collaborator

0crat commented Apr 23, 2019

Job #1114 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Apr 23, 2019

This pull request #1114 is assigned to @ilyakharlamov/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

@@ -62,26 +62,52 @@ public BehavesAsCollection(final E item) {
@SuppressWarnings({ "unchecked", "PMD.ClassCastExceptionWithToArray" })
public boolean matchesSafely(final Collection<E> col) {
MatcherAssert.assertThat(
col, new IsCollectionContaining<>(new IsEqual<>(this.sample))
"Must contain item",
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though MatcherAssert.assertThat is still used, we are in transition to OO/Junit5 new Assertion for all the new code added.
Please use new Assertion<>().affirm()

@@ -54,6 +54,7 @@ public void behavesAsCollection() throws Exception {
@Test
public void filterList() {
MatcherAssert.assertThat(
"Length must change",
Copy link
Contributor

Choose a reason for hiding this comment

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

new Assertion<>().affirm()

Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah message should be "length equal to number of items matching filter"

Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah actually I don't understand what the difference is between this test and the size() test below. I think this one can be deleted

@@ -79,6 +83,7 @@ public void decoratesArray() throws Exception {
@Test
public void testEmpty() {
MatcherAssert.assertThat(
"Must be empty",
Copy link
Contributor

Choose a reason for hiding this comment

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

new Assertion<>().affirm()

Matchers.contains(
"three",
"four"
new IsCollectionContaining<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

throws Exception is not needed here

@vladhss
Copy link
Contributor Author

vladhss commented Apr 23, 2019

@ilyakharlamov I replaced all MatcherAssert.assertThat with new Assertion<>().affirm() and removed throws Exception where it is not needed, please check

@ilyakharlamov
Copy link
Contributor

@llorllale it's good to merge

"two",
"three"
new IsCollectionContaining<>(
new AnyOf<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@Umbrah are you sure the logic is preserved here? Why not use AllOf?

new Assertion<>(
"Must contain item",
() -> col,
new IsCollectionContaining<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah can't you use HasValues?

new Assertion<>(
"Size must be more than 0",
() -> col,
new IsCollectionWithSize<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah HasSize

() -> new ListOf<>(
(E[]) col.toArray()
),
new IsCollectionContaining<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah HasValues

new Assertion<>(
"Array from collection must contain item",
() -> new ListOf<>(array),
new IsCollectionContaining<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah HasValues

new Filtered<String>(
public void size() {
new Assertion<>(
"Size must change",
Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah message must be "size is equal to number of items matching the filter"

Matchers.arrayContaining(4, 3, 2, 1)
);
new IsArrayContaining<>(
new AnyOf<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah AllOf?

Matchers.arrayContaining(5, 4, 3, 2, 1)
);
new IsArrayContaining<>(
new AnyOf<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah AllOf?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Umbrah better yet, HasValues

Matchers.contains(
"three",
"four"
new IsCollectionContaining<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@umbrah HasValues

@llorllale
Copy link
Contributor

@umbrah see above.

Basically I worry that the logic may not have translated correctly into the new refactored code. Also, take a look at the available matchers from cactoos-matchers - will probably make your life easier in most cases.

@vladhss
Copy link
Contributor Author

vladhss commented Apr 24, 2019

@llorllale The logic was incorrect, I updated all usages of IsCollectionContaining<>(AnyOf<>(...)) with AllOf<>(IsCollectionContaining<>(...)). I cannot use HasValues and HasSize as we do not have it in last published version of cactoos-matchers. Please check.

@llorllale
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Apr 25, 2019

@rultor merge

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

@rultor rultor merged commit 622165a into yegor256:master Apr 25, 2019
@rultor
Copy link
Collaborator

rultor commented Apr 25, 2019

@rultor merge

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

@0crat
Copy link
Collaborator

0crat commented Apr 25, 2019

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

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

0crat commented Apr 25, 2019

@sereshqua/z please review this job completed by @ilyakharlamov/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 25, 2019

Job #1114 is not in the agenda of @ilyakharlamov/z, can't inspect

@0crat
Copy link
Collaborator

0crat commented Apr 25, 2019

The job #1114 is now out of scope

@0crat
Copy link
Collaborator

0crat commented Apr 25, 2019

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

@sereshqua
Copy link

@0crat quality good

@0crat
Copy link
Collaborator

0crat commented Apr 25, 2019

Order was finished, quality is "good": +25 point(s) just awarded to @ilyakharlamov/z

@0crat
Copy link
Collaborator

0crat commented Apr 25, 2019

Quality review completed: +4 point(s) just awarded to @sereshqua/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.

7 participants