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

Add not() matcher operator #84

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bendykowski
Copy link
Collaborator

Co-authored-by: cakeinpanic [email protected]

Co-authored-by: cakeinpanic <[email protected]>
@codecov-io
Copy link

codecov-io commented Feb 9, 2018

Codecov Report

Merging #84 into master will decrease coverage by 0.12%.
The diff coverage is 95.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
- Coverage   94.58%   94.45%   -0.13%     
==========================================
  Files          35       36       +1     
  Lines         609      667      +58     
  Branches       69       72       +3     
==========================================
+ Hits          576      630      +54     
- Misses         24       25       +1     
- Partials        9       12       +3
Impacted Files Coverage Δ
src/matcher/type/AnyOfClassMatcher.ts 100% <100%> (ø) ⬆️
src/ts-mockito.ts 96.82% <100%> (+0.1%) ⬆️
src/matcher/type/ObjectContainingMatcher.ts 90% <100%> (ø) ⬆️
src/matcher/ArgsToMatchersValidator.ts 100% <100%> (ø) ⬆️
src/matcher/type/DeepEqualMatcher.ts 80% <100%> (ø) ⬆️
src/matcher/type/AnyStringMatcher.ts 88.88% <100%> (ø) ⬆️
src/matcher/type/Matcher.ts 66.66% <100%> (ø) ⬆️
src/matcher/type/AnythingMatcher.ts 87.5% <100%> (ø) ⬆️
src/matcher/type/AnyFunctionMatcher.ts 100% <100%> (ø) ⬆️
src/matcher/type/BetweenMatcher.ts 90.9% <100%> (ø) ⬆️
... and 8 more

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 ba5997f...66da980. Read the comment docs.

@bendykowski
Copy link
Collaborator Author

Hi @cakeinpanic, I've created another PR with my changes. I thought it will be the fastest way. You are added as a co-author of those changes of course. I hope @NagRock will be able to check that soon.

@cakeinpanic
Copy link
Contributor

Are you sure package.lock is needed to be in repo?

@cakeinpanic
Copy link
Contributor

What is the difference between my PR and your except of changes in Mock.ts?

@bendykowski
Copy link
Collaborator Author

bendykowski commented Feb 12, 2018

Hi @cakeinpanic, package-lock.json indeed should be a part of a repo (see: https://docs.npmjs.com/files/package-lock.json). The difference is that I've changed the not mechanism. I've renamed it to NotOperator which better describes what it is. Also removed all unnecessary code from all matchers. Instead of changing matchers themselves, adding everywhere handling functionality I've moved it to NotOperator. For now NotOperator, when ex. anyFunction() is called on it, creates AnyFunctionMatcher, saves it and returns itself (not AnyFunctionMatcher). When the stubbing mechanism is checking if a param is matching a given matcher, calls isMatching() method on NotOperator which calls isMatching() on a saved matcher and reverts the output. The same when the pretty name is needed, NotOperator calls toString() on a saved matcher and prepend the output with not().. Doing that we are separating the logic of matchers and operators, don't have to change matchers and remember to add this logic when creating a new matcher. To achieve that, still extending the Matcher class, as I need the same interface, I had to rename core match() method to isMatching() to avoid a conflict with MatchMatcher and better describe the purpose of this method. I've kept the rest of your changes intact.

@cakeinpanic
Copy link
Contributor

@bendykowski okey, now I see the difference, thank you. Your solution looks better

The only one thing you forgot is toString, method for not() . matcher, I think it might be useful sometimes

@cakeinpanic cakeinpanic mentioned this pull request Feb 13, 2018
@cakeinpanic
Copy link
Contributor

@NagRock friendly reminder for review/merge)

@cakeinpanic
Copy link
Contributor

@NagRock friendly reminder for review/merge)

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.

3 participants