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

(#165) Rename Verifies to Satisfies and improve API #242

Merged
merged 1 commit into from
May 2, 2021

Conversation

victornoel
Copy link
Collaborator

This is a followup on #165 to improve Verifies.

  • It was renamed to Satisfies to match the naming using in other similar tooling such as assertj.
  • The API was improved to support any type of feature extraction

@victornoel
Copy link
Collaborator Author

@0crat assign @andreoss

@codecov
Copy link

codecov bot commented May 2, 2021

Codecov Report

Merging #242 (181db92) into master (da75230) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #242      +/-   ##
============================================
+ Coverage     98.91%   98.95%   +0.03%     
- Complexity      147      150       +3     
============================================
  Files            28       28              
  Lines           370      383      +13     
  Branches          7        7              
============================================
+ Hits            366      379      +13     
  Misses            4        4              
Impacted Files Coverage Δ Complexity Δ
...java/org/llorllale/cactoos/matchers/Satisfies.java 100.00% <100.00%> (ø) 6.00 <5.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 da75230...181db92. Read the comment docs.

@0crat
Copy link
Collaborator

0crat commented May 2, 2021

Job #242 is already in scope

func,
desc -> desc.appendText("verifies"),
(obj, desc) -> desc.appendText("was ").appendValue(obj)
obj -> matcher.matches(extractor.apply(obj)),
Copy link
Contributor

Choose a reason for hiding this comment

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

@victornoel Besides the bug-prone double calling of 'apply', it looks fine.

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 yep, we can discuss this double-calling in #239 to see if we can find some idea on how to improve this

@victornoel
Copy link
Collaborator Author

@rultor merge

@rultor
Copy link
Collaborator

rultor commented May 2, 2021

@rultor merge

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

@rultor rultor merged commit 181db92 into llorllale:master May 2, 2021
@rultor
Copy link
Collaborator

rultor commented May 2, 2021

@rultor merge

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

@0crat 0crat added the qa label May 2, 2021
@0crat
Copy link
Collaborator

0crat commented May 2, 2021

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

@0crat 0crat removed the 0crat/scope label May 2, 2021
@0crat
Copy link
Collaborator

0crat commented May 2, 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

@sereshqua
Copy link

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

@sereshqua
Copy link

@0crat quality acceptable

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