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

Simplify TextMatcher #204

Merged
merged 2 commits into from
Sep 29, 2020
Merged

Simplify TextMatcher #204

merged 2 commits into from
Sep 29, 2020

Conversation

victornoel
Copy link
Collaborator

This is for #203. I also simplified the code of of Assertion, I'm not sure why it was so complex but now at least it is easier to use the debugger with it.

@codecov
Copy link

codecov bot commented Sep 27, 2020

Codecov Report

Merging #204 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #204   +/-   ##
=========================================
  Coverage     98.71%   98.71%           
+ Complexity      133      130    -3     
=========================================
  Files            26       26           
  Lines           311      311           
  Branches          3        3           
=========================================
  Hits            307      307           
  Misses            4        4           
Impacted Files Coverage Δ Complexity Δ
...java/org/llorllale/cactoos/matchers/Assertion.java 100.00% <100.00%> (ø) 3.00 <2.00> (-3.00)
.../java/org/llorllale/cactoos/matchers/EndsWith.java 100.00% <100.00%> (ø) 3.00 <1.00> (ø)
...a/org/llorllale/cactoos/matchers/MatchesRegex.java 100.00% <100.00%> (ø) 3.00 <1.00> (ø)
...ava/org/llorllale/cactoos/matchers/StartsWith.java 100.00% <100.00%> (ø) 3.00 <1.00> (ø)
.../org/llorllale/cactoos/matchers/TextHasString.java 100.00% <100.00%> (ø) 3.00 <1.00> (ø)
...in/java/org/llorllale/cactoos/matchers/TextIs.java 100.00% <100.00%> (ø) 3.00 <1.00> (ø)
...va/org/llorllale/cactoos/matchers/TextMatcher.java 100.00% <100.00%> (ø) 4.00 <0.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 c5404b8...f834f8c. Read the comment docs.

return new AssertionError(text.toString());
}
);
this.msg = msg;
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornoel Shouldn't the names of fields and ctor parameters be different?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andreoss some people seems to think it is important, but since I am ARC, I've never enforced that for anybody because I don't think it is useful or has any advantage, the opposite actually, it forces you to invent new names for the same thing...
Why do you think it should be the case? I could change my mind

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andreoss good find!! thanks 👍 :)

@victornoel
Copy link
Collaborator Author

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Sep 29, 2020

@rultor merge

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

@rultor rultor merged commit f834f8c into llorllale:master Sep 29, 2020
@rultor
Copy link
Collaborator

rultor commented Sep 29, 2020

@rultor merge

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

@victornoel victornoel deleted the 203 branch September 29, 2020 14:13
@0crat 0crat added the qa label Sep 29, 2020
@0crat
Copy link
Collaborator

0crat commented Sep 29, 2020

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

@0crat 0crat removed the 0crat/scope label Sep 29, 2020
@0crat
Copy link
Collaborator

0crat commented Sep 29, 2020

@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

@sereshqua
Copy link

@andreoss please make sure you will find at least 3 issues during next CR

@sereshqua
Copy link

@victornoel i will give a verdict straightaway, cause last couple of issues i haven't received any response or reaction on QA quality reminder :) sorry in advance

@sereshqua
Copy link

@0crat quality acceptable

@victornoel
Copy link
Collaborator Author

@sereshqua hehe yes, I saw that. I think some of the DEV are used to the reminders ^^ luckily, there are some new members around in cactoos that I'm sure will be happy to get feedback from QA! Keep the good work ;)

@sereshqua
Copy link

@victornoel thanks :)

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.

6 participants