Negating the matcher #15

hieueastagile · 2016-11-15T20:04:45Z

Description

Add not_to syntax

Note

This PR is in-progress and for proof of concept only. Further refactor needed to make it final.
There're a few issues that might need to address before it could be an official version:
- How should the failure message be changed?
- Any other should be affected?
- A lot of integration tests should be added
- Double negated matcher?

Relevant issues

Negate the matcher #13 should be resolved when this PR is merged.

hieueastagile · 2016-11-15T20:05:22Z

@tklarryonline @steadykhoi @nhanbt @hotay please help taking a look on this one.

coveralls · 2016-11-15T20:06:01Z

Coverage increased (+0.006%) to 99.662% when pulling b59b393 on feature/expect-not into de515f3 on master.

anhhuy1605 · 2016-11-16T01:50:45Z

robber/expect.py

+        self.to = self
+        self.be = self
+        self.a = self
+        self.an = self
        self.have = self


Should we have self.is_negated = False for each property here? 🤔

@anhhuy1605 Not yet, I think. I will think about it again when I consider about the double (triple...) negated matcher.

I think there should be a not property that trigger is_negated as well.

We should have a function something like negate to toggle the is_negated to handle the double/triple... negating as well, it will come in the next commit.

steadykhoi · 2016-11-16T07:07:02Z

robber/expect.py

-                    .match()
+                                            klass(self.obj, other, self.is_negated, *args) \
+                                            .fail_with(self.message) \
+                                            .match()


What's with the outrageous indentation?

I think this indentation is correct, the previous code of robber.py didn't use PEP8 or flake8 and I fixed this when I see the warnings.

I think it's better to declare a method here, rather than making a big lambda like this...

@tklarryonline It's old code, I just keep it the same to make the proof of concept work first. Will do the refactor later.

steadykhoi · 2016-11-16T07:09:31Z

robber/expect.py

+    @property
+    def not_to(self):
+        self.is_negated = True
+        return self


if we have a not property then this is not needed. Since you could just write .not.to.

not is Python keyword, I think that we should avoid it.

steadykhoi · 2016-11-16T07:11:13Z

tests/test_integration.py

+        expect(1).not_to.eq(2)
+        expect([1, 2]).not_to.eq([2, 1])
+        expect((1, 2)).not_to.eq((2, 1))
+        expect(1).not_to == 2


The grammatically correct way to write these is:

expect(<something>).to.not.eq(<something>)

to_not and not_to are both correct, but not_to is more popular, see the discussion here: #13

Haha, I was just about to say the same thing.

tklarryonline · 2016-11-16T07:44:49Z

robber/matchers/base.py

        self.args = args

    def fail_with(self, message):
        self.message = message
        return self

    def match(self):
-        if not self.matches():
+        if not self.matches() and not self.is_negated:
            message = self.message or self.failure_message()
            raise BadExpectation(message)


How about self.matches() and self.is_negated?

It should be, I made it quick to see if it works, the big refactor will be coming to address the remaining issues that I listed in the PR description.

coveralls · 2016-11-16T13:36:03Z

Coverage increased (+0.008%) to 99.664% when pulling b331815 on feature/expect-not into de515f3 on master.

catriuspham · 2016-11-17T08:01:47Z

.gitignore

@@ -34,3 +34,5 @@ nosetests.xml
 .mr.developer.cfg
 .project
 .pydevproject
+.python-version
+.idea/


Missing a blank line here

Great to see your comment here @catriuspham. Yes, you are right, I will add it soon.

catriuspham · 2016-11-17T08:09:30Z

tests/matchers/test_numbers.py

-        expect(Within(1, 0, 2).matches()) == True
-        expect(Within(2, 3, 4).matches()) == False
+        expect(Within(1, 0, False, 2).matches()) == True
+        expect(Within(2, 3, False, 4).matches()) == False


I think the boolean value should be at the end or the beginning of the variable set. Like:
expect(Within(1, 0, 2, False).matches()) == True or expect(Within(False, 1, 0, 2).matches()) == True

@catriuspham You are right, but I think we can't do it here. Because this Within was extended from Base, which is actually defined like this:

class Base: def __init__(actual, expected, is_negated, *args): pass

The last value of Within is actually the agrs[0], then we can't control it.

catriuspham · 2016-11-17T08:12:45Z

robber/matchers/boolean.py

@@ -11,7 +11,9 @@ def matches(self):
        return self.actual == True

    def failure_message(self):
-        return 'Expected %s to be True' % self.actual
+        negated_message = ' not' if self.is_negated else ''
+        return 'Expected %s%s to be True' % (self.actual, negated_message)


how about moving the space to between the %s, like this:

negated_message = 'not' if self.is_negated else '' return 'Expected %s %s to be True' % (self.actual, negated_message)

If we move the space into the %s, when we don't have not_to, it will be something like this:

Expected False to be true

When we just want this:

Expected False to be true

oyster · 2016-12-05T03:14:19Z

Please resolve the conflicts for this pull request. Thanks.

hieueastagile · 2016-12-05T04:01:35Z

@oyster This PR is not mergeable yet, we still have a lot of things to do, I will update it in the PR description later.

oyster · 2016-12-05T04:12:40Z

Thanks, please update and add a label like WIP to this pull request.

hieueastagile · 2016-12-05T04:58:44Z

@oyster I had that intention, but it looks like I don't have enough permission to do so.

oyster · 2016-12-05T07:45:21Z

Ok, I created a new label WIP and added it here.

tklarryonline · 2016-12-22T04:51:06Z

How is it now, @hieueastagile ?

hieueastagile · 2016-12-22T05:47:03Z

@tklarryonline Still have no time for it :(. It still had a lot of things to do with the failure_message.

catriuspham · 2017-01-11T03:39:29Z

I'm kind of free now, how can I contribute?

tklarryonline · 2017-01-11T03:48:22Z

@catriuspham Let's start with building this repo locally :)

hieueastagile · 2017-01-11T04:12:12Z

@catriuspham Let's start with set-up this repo in your local as @tklarryonline said. After that, you can look up to the issues, there's a lot of available stuff you can do there. If you need any help, feel free to raise it at #python channel.

catriuspham · 2017-01-11T08:36:49Z

OK 😄

- Move negated_message into base - Remove unused __str__ function in BadExpectation - Use string format in failure massage - Change "to not" to "not to"

coveralls · 2017-03-26T20:10:48Z

Coverage increased (+0.3%) to 100.0% when pulling b7838e0 on feature/expect-not into 576f050 on master.

Now we can register a negative matcher using the positive one. The failure message need to be customized if needed. The benefit is that we don't have to create a whole new class for those negative matchers. - Replace False matcher with negative True matcher - Replace Exclude/NotContain matcher with negative Contain matcher - Replace Falsy matcher with negative Truthy matcher

hieueastagile · 2017-04-06T01:38:15Z

robber/expect.py

-        self.object = object
+    def __init__(self, obj):
+        self.obj = obj
+        self.is_negative = False


Hm, I guess is_negated is actually a better name, guy @catriuspham :P.

@hieueastagile Could you explain why? As I mentioned in this commit de70d8c

Base on OALD, "negative" means "containing a word such as 'no', 'not',
'never', etc.". Moreover, I cannot find the word "negated".

@catriuspham See this definition: http://www.dictionary.com/browse/negate. negated is closer to the meaning of what we are going to do. I think you can find it if you take a look around to another assertion library also.

@hieueastagile well, you can take a look at the fourth meaning in OALD: http://www.oxfordlearnersdictionaries.com/definition/english/negative_1?q=negative.
It literally means "containing a word such as ‘no’, ‘not’, ‘never’, etc.".
Another source is Google Trends: https://trends.google.com/trends/explore?q=negative,negated
You can see that "negative" is more widely used, while the usage of "negated" is really limited.

I don't say about the trend here, since negate is not very popular, of course. But the meaning of it is something like the reversing the matcher's meaning, which is obviously closer to what we are going to do. Because logically, you can do something like not_to.not_to, it contains not but actually it's not negative, it's just negated twice. Anyway, it's not too bad then.

hieueastagile · 2017-04-06T01:39:13Z

robber/expect.py

        cls.matchers[name] = klass

        def method(self, other=None, *args):
-            return klass(self.object, other, *args).fail_with(self.message).match()
+            fact = is_negative is not self.is_negative


What do you really want to check here @catriuspham ? is_negative != self.is_negative?

Let's take a look at how I registered the boolean matchers.

expect.register('true', TrueMatcher) expect.register('false', TrueMatcher, is_negative=True)

self.is_negative is the initial nature of the matcher. As for the TrueMatcher here, it it False.
is_negative is how we want to use it. As for the TrueMatcher registered as false here, it it True.
is_negative != self.is_negative decide the final nature of the matcher. I will put a fact table here:

self.is_negative is_negative Fact

True True False

True False True

False True True

False False False

From this fact table, I got that formula.

Hm, I rather write a FalseMatcher to make it cleaner than re-use the TrueMatcher in this case. It seems magical to me. Anyway, do it your choice then.

Well, I agree with you that it's (really) magical. But only the implementation is complicated, and we only have to do it once. This way of registration is really easy to use, DRY and saves time. So in my opinion, the trade-off is acceptable.

You can do it your way, but FYI, explicit/easy to read/easy to understand is always prefer than DRY, especially for python code.

hieueastagile · 2017-04-06T01:42:28Z

@catriuspham Could you do a list what you have finished and which not? (please ignore it if it takes you a lot of time). Then I can do help if I have some free time.

catriuspham · 2017-04-06T03:20:50Z

@hieueastagile Actually, I have finished all the not expectation ^^ Please help review all the commits.

coveralls · 2017-04-06T11:05:39Z

Coverage increased (+0.3%) to 100.0% when pulling 44edf78 on feature/expect-not into 54d5657 on master.

catriuspham · 2017-04-06T11:06:23Z

@hieueastagile As I have finished working on this PR, please help change the PR title into something like:

[f] Negative expectations

And remove the WIP tag also

hieueastagile · 2017-04-06T14:50:27Z

Negating the matcher is the correct title, I think we don't need to change it. About the WIP, you need to ask @oyster.

Anyway, there's a case the I didn't see it here: Double negated matcher, if we plan to not allow it, I think we should raise the exception for the case: not_to.not_to or the case with more not_to.

tklarryonline · 2017-04-06T15:02:50Z

@hieueastagile I thought you agreed to let it be out there for fun...

But yeah, that would lead to an overflow (in some case) so I think we could raise an exception for that. ISeeWhatYouDidThereException...

hieueastagile · 2017-04-06T15:17:50Z

lol, up to @catriuspham then @tklarryonline

catriuspham · 2017-04-07T10:02:47Z

@hieueastagile About the title, I mean we don't need that "First draft version" anymore, as this PR becomes really "serious" now 😂

catriuspham · 2017-04-07T10:12:06Z

@tklarryonline I tried to support the "fun stuff" but then realized that it was not so easy so... My apologies...

- not_to.not_to.not_to.not_to.not_to.not_to.eq(something) is available - rewrite the registration logic to make it easier to understand

catriuspham · 2017-04-08T00:07:32Z

@tklarryonline I found out an easy way to make multiple negative chains work 😛

coveralls · 2017-04-08T00:07:39Z

Coverage increased (+0.3%) to 100.0% when pulling 9dbecb7 on feature/expect-not into 54d5657 on master.

tklarryonline · 2017-04-08T03:54:32Z

@catriuspham Great to know! 😀 Anyhow, I think @hieueastagile doesn't really like it... But yeah it's up to you hehe

tklarryonline · 2017-04-08T03:58:45Z

tests/test_expect.py

+
+        expect(1).not_to.ne(2)
+        expect(1).not_to.not_to.not_to.ne(2)
+        expect(1).not_to.not_to.not_to.not_to.not_to.ne(2)


Have to admit that reading this cause my little brain blows 😂, but it's fun figuring these chains.

Legend says that if you read it out loud, you gonna become a rapper 🙄

tklarryonline · 2017-04-08T04:02:47Z

robber/matchers/boolean.py



 expect.register('true', TrueMatcher)
-expect.register('false', FalseMatcher)
+expect.register('false', TrueMatcher, is_negative=True)


I think the name TrueMatcher is quite confusing as this class also supports False cases...

- Rename TrueMatcher to Boolean

catriuspham · 2017-04-10T08:30:14Z

@NhanHo I think you want to take a look :)

coveralls · 2017-04-10T08:37:14Z

Coverage increased (+0.3%) to 100.0% when pulling 73155be on feature/expect-not into 54d5657 on master.

hieueastagile · 2017-04-13T04:24:53Z

@NhanHo please take a look on this soon. Thanks

hieueastagile · 2017-04-19T04:45:54Z

@oyster please help merging this.

First draft version for negated matcher

b59b393

hieueastagile changed the title ~~First draft version for negated matcher~~ First draft version for negating the matcher Nov 15, 2016

anhhuy1605 reviewed Nov 16, 2016

View reviewed changes

steadykhoi reviewed Nov 16, 2016

View reviewed changes

tklarryonline reviewed Nov 16, 2016

View reviewed changes

Revise boolean matcher failure message with not_to syntax

b331815

catriuspham reviewed Nov 17, 2016

View reviewed changes

oyster added the WIP label Dec 5, 2016

ea-open-source added 3 commits March 24, 2017 12:12

Merge branch 'master' into feature/expect-not

a43b712

[f] Rewrite base

6f6e9a1

- Move negated_message into base - Remove unused __str__ function in BadExpectation - Use string format in failure massage - Change "to not" to "not to"

[f] Add support for not.to.be.callable

b7838e0

ea-open-source added 2 commits April 5, 2017 02:04

[f] Add support for not_to.contain

ce613f5

hieueastagile commented Apr 6, 2017

View reviewed changes

Merge branch 'master' into feature/expect-not

44edf78

oyster removed the WIP label Apr 7, 2017

hieueastagile changed the title ~~First draft version for negating the matcher~~ Negating the matcher Apr 7, 2017

[f] Add support for multiple negative chain

9dbecb7

- not_to.not_to.not_to.not_to.not_to.not_to.eq(something) is available - rewrite the registration logic to make it easier to understand

tklarryonline reviewed Apr 8, 2017

View reviewed changes

[r] Add not_to to guide

73155be

- Rename TrueMatcher to Boolean

This was referenced Apr 10, 2017

100% test coverage #6

Closed

Negate the matcher #13

Closed

oyster merged commit a7341f6 into master Apr 19, 2017

Negating the matcher #15

Negating the matcher #15

Conversation

hieueastagile commented Nov 15, 2016 • edited Loading

Description

Note

Relevant issues

hieueastagile commented Nov 15, 2016

coveralls commented Nov 15, 2016 • edited Loading

anhhuy1605 Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catriuspham Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catriuspham Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

hieueastagile Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

oyster commented Dec 5, 2016

hieueastagile commented Dec 5, 2016

oyster commented Dec 5, 2016

hieueastagile commented Dec 5, 2016

oyster commented Dec 5, 2016

tklarryonline commented Dec 22, 2016

hieueastagile commented Dec 22, 2016

catriuspham commented Jan 11, 2017

tklarryonline commented Jan 11, 2017

hieueastagile commented Jan 11, 2017

catriuspham commented Jan 11, 2017

coveralls commented Mar 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catriuspham Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catriuspham Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieueastagile commented Apr 6, 2017

catriuspham commented Apr 6, 2017

coveralls commented Apr 6, 2017 • edited Loading

catriuspham commented Apr 6, 2017

hieueastagile commented Apr 6, 2017

tklarryonline commented Apr 6, 2017

hieueastagile commented Apr 6, 2017

catriuspham commented Apr 7, 2017

catriuspham commented Apr 7, 2017

catriuspham commented Apr 8, 2017

coveralls commented Apr 8, 2017 • edited Loading

tklarryonline commented Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catriuspham commented Apr 10, 2017

coveralls commented Apr 10, 2017 • edited Loading

hieueastagile commented Apr 13, 2017

hieueastagile commented Apr 19, 2017

hieueastagile commented Nov 15, 2016 •

edited

Loading

coveralls commented Nov 15, 2016 •

edited

Loading

anhhuy1605 Nov 16, 2016 •

edited

Loading

coveralls commented Nov 16, 2016 •

edited

Loading

catriuspham Nov 17, 2016 •

edited

Loading

catriuspham Nov 17, 2016 •

edited

Loading

hieueastagile Nov 17, 2016 •

edited

Loading

catriuspham Apr 7, 2017 •

edited

Loading

catriuspham Apr 6, 2017 •

edited

Loading

coveralls commented Apr 6, 2017 •

edited

Loading

coveralls commented Apr 8, 2017 •

edited

Loading

tklarryonline commented Apr 8, 2017 •

edited

Loading

coveralls commented Apr 10, 2017 •

edited

Loading