-
Notifications
You must be signed in to change notification settings - Fork 21
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
Simplify TextMatcher #204
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
return new AssertionError(text.toString()); | ||
} | ||
); | ||
this.msg = msg; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victornoel I don't think this rule is important either.
It was removed last year https://github.com/teamed/qulice/blame/e296098d1722f13ad6644f6a2958b5e8d1cbc47c/qulice-checkstyle/src/main/resources/com/qulice/checkstyle/checks.xml#L201
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreoss good find!! thanks 👍 :)
@rultor merge |
@victornoel OK, I'll try to merge now. You can check the progress of the merge here |
@victornoel Done! FYI, the full log is here (took me 9min) |
Job was finished in 43 hours, bonus for fast delivery is possible (see §36) |
@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 |
@andreoss please make sure you will find at least 3 issues during next CR |
@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 |
@0crat quality acceptable |
@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 ;) |
@victornoel thanks :) |
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.