Implementation of equality for Settings class by bhavishyagopesh · Pull Request #464 · python-hyper/h2

bhavishyagopesh · 2017-02-09T16:00:35Z

I have defined __eq__ and __ne__ for Settings class, and wrote tests for the same.

Lukasa

Ok, good start! There are some stylistic problems here, but we're well on our way. The biggest problems are going to be on the testing front.

I've left more detailed notes below.

Lukasa · 2017-02-09T16:03:14Z

h2/settings.py



+    def __eq__(self,other):
+        return self._settings==other._settings


Style nit: we need a space either side of the operator.

I should also note that this does not behave correctly when compared with objects that are not Settings objects: specifically, it'll probably throw AttributeErrors.

To deal with this, you need to confirm that the other object is an instance of Settings before doing the comparison. If it's not, you want to indicate that we don't know how to compare them by returning NotImplemented.

Lukasa · 2017-02-09T16:03:22Z

h2/settings.py

        return len(self._settings)


+    def __eq__(self,other):


Style nit: space after comma.

Lukasa · 2017-02-09T16:03:28Z

h2/settings.py

+    def __eq__(self,other):
+        return self._settings==other._settings
+
+    def __ne__(self,other):


Style nit: space after comma.

Lukasa · 2017-02-09T16:03:52Z

h2/settings.py

+        return self._settings==other._settings
+
+    def __ne__(self,other):
+        return not self._settings==other._settings


So it's usually best to implement this in terms of equality unless there's a good reason not to, so I'd write this as return not self == other.

Maybe even skip the middle-man and call return not self.__eq__(other) :)

Actually, you don't want to do that. ;)

The reason is that the rich equality tools in Python have some complex logic, and we don't want to privilege our own execution order. It's better to let Python work it out then to say "you must look at this specific method first".

Lukasa · 2017-02-09T16:04:06Z

h2/settings.py

+
+
+
+


We don't want to add all this whitespace.

Lukasa · 2017-02-09T16:04:22Z

test/test_h2_upgrade.py

        assert (
            client.local_settings._settings == server.remote_settings._settings
-        )
+        )


You've made some changes in here that are only whitespace: please remove them.

Lukasa · 2017-02-09T16:04:31Z

test/test_settings.py

            assert s.max_header_list_size is None

-            with pytest.raises(h2.exceptions.InvalidSettingsValueError) as e:
+            with pytest.raises(h2.exceptions.InvalidSettingsValueError) as e: 


Please remove the trailing whitespace from here.

Lukasa · 2017-02-09T16:08:19Z

test/test_settings.py

+    def notequal_objects_are_notequal(s1,s2):
+        s1.acknowledge()
+        s2.acknowledge()
+        assert s1._settings!=s2._settings


Ok, so these tests cannot possibly work right now for a number of reasons. Let's discuss why.

Firstly, and most importantly, where are s1 and s2 coming from? Nowhere else in the code knows how to provide them. If you look at tests higher up the file you'll see they create their own Settings objects: your tests need to do that as well.

Secondly, you need to think a bit harder about what these tests are actually testing. What you want to do is to confirm that the code you just wrote actually works. That means you want to test that two Settings entities that are the same are equal, and two that are different are not equal.

As an example, the simplest test that meaningfully tests something looks like this:

def test_newly_created_settings_are_equal(self): s1 = h2.settings.Settings() s2 = h2.settings.Settings() assert s1 == s2

Note as well some basic rules of Python code: specifically, note that these test methods are methods, and so must take a self parameter, and note as well that to have them count as a test they need to have names that begin withtest_.

I have done most of the changes as you said but , tox is showing AssertionError with

"assert not s1 == s2"
or

"assert s1 != s2"
tox shows this error
assert <h2.settings.Settings object at 0x10a749410> != <h2.settings.Settings object at 0x10a749490>

sethmlarson · 2017-02-09T20:40:31Z

h2/settings.py

+        if isinstance(other, Settings):
+            return self._settings == other._settings
+        else:
+            return NotImplemented


If other isn't a Settings object we can pretty easily say we're not equal. Should return False in this case.

Sorry @SethMichaelLarson, but again I disagree. =) Per the documentation, NotImplemented should be returned when we have no method of determining equality with respect to the other type. This is to allow other rich comparison methods to potentially be invoked that may know how to do an equality comparison.

The correct thing to do here is to return NotImplemented when we don't have matching types.

@Lukasa Huh! TIL that NotImplemented exists. I never knew of this construct. I will keep this in mind! Thanks! :)

sethmlarson · 2017-02-09T20:40:45Z

h2/settings.py

+        if isinstance(other, Settings):
+            return self._settings != other._settings
+        else:
+            return NotImplemented


Same as above, except True.

sethmlarson · 2017-02-09T20:42:30Z

h2/settings.py

@@ -254,12 +254,18 @@ def __len__(self):
        return len(self._settings)




This whitespace should probably get removed. Only supposed to be 1 line between class methods.

sethmlarson · 2017-02-09T21:17:29Z

@bhavishyagopesh Okay, my suggestions are fulfilled, now go through @Lukasa's change requests.

alexwlchan

Two comments, and I’ll provide feedback on the tests once they’re a bit more fleshed out.

FWIW, this EqualityTestsMixin class from the pyOpenSSL tests has pretty good coverage of the sort of testing I’d expect to see of __eq__ and __ne__ methods: https://github.com/pyca/pyopenssl/blob/b748099426db3963c7b06d2e0aec29e12e016c07/tests/util.py#L35

alexwlchan · 2017-02-09T21:34:56Z

h2/settings.py

+            return False
+
+    def __ne__(self, other):
+        if isinstance(other, Settings):


I’d push for

return not self == other

unless there’s a good reason not to, as otherwise we’re duplicating logic from the __eq__ method.

I’d go one step further and drop the isinstance() call – we’re still duplicating that. Simply:

def __ne__(self, other): return not self == other

Yup, completely agreed with @alexwlchan here.

alexwlchan · 2017-02-09T21:35:08Z

test/test_h2_upgrade.py

        assert (
            client.local_settings._settings == server.remote_settings._settings
        )
+


Please revert these unrelated whitespace changes.

My requested changes are completed.

alexwlchan · 2017-02-10T09:09:57Z

test/test_h2_upgrade.py

        assert (
            client.local_settings._settings == server.remote_settings._settings
-        )
+        ) 


We still have whitespace changes creeping into this file.

There are still whitespace changes here that need removing.

Lukasa · 2017-02-10T09:23:31Z

h2/settings.py

-
+    def __eq__(self, other):
+        if isinstance(other, Settings):
+            return self == other


So, this doesn't work because it leads to a recursive definition. Specifically, it says self is equal to other if self equals other. That's tautologically accurate, but doesn't help Python to actually work out what equality looks like.

This should be changed to specifically compare the _settings properties of the two objects.

Lukasa · 2017-02-10T09:23:43Z

h2/settings.py

+            return False
+
+    def __ne__(self, other):
+        if isinstance(other, Settings):


Yup, completely agreed with @alexwlchan here.

bhavishyagopesh · 2017-02-10T22:03:20Z

@SethMichaelLarson I was asking for values of initial_values parameter.

sethmlarson · 2017-02-10T22:06:13Z

@bhavishyagopesh Look around the other tests within test_settings.py for some good ideas on values to use.

bhavishyagopesh · 2017-02-10T23:46:09Z

@SethMichaelLarson I gave some values to initial_values but still it doesn't cover line 257-260 and 263.

bhavishyagopesh · 2017-02-11T00:02:29Z

@Lukasa I guess, I have corrected linting errors, now what changes(additional tests to be added...) need to be made to cover line 257-260 and 263 in settings.py

Lukasa

The reason you are missing coverage is because these tests aren't actually running, as the class name doesn't have the right form. Making that change should get your coverage up.

Lukasa · 2017-02-11T12:17:37Z

test/test_settings.py

                s[h2.settings.MAX_HEADER_LIST_SIZE]
+
+
+class test_settings_equality(object):


This class name should be TestSettingsEquality.

bhavishyagopesh · 2017-02-11T12:54:02Z

@Lukasa Comparing two lists return assertionerror with ne but not with eq ?

bhavishyagopesh · 2017-02-11T14:44:04Z

@SethMichaelLarson Comparing two lists return assertionerror with ne but not with eq ?

bhavishyagopesh · 2017-02-11T16:33:38Z

@Lukasa If you could cite some resource for what's really happening?Why it isn't passing?

bhavishyagopesh · 2017-02-11T19:11:27Z

Is anyone there who would be kind enough to tell the error. Please......

Lukasa

@bhavishyagopesh Please be patient. You aren't being ignored: the developers who can review your work are all busy people who have jobs and other parts of their lives they need to attend to, and cannot necessarily respond quickly to you. Remain patient, and trust that you aren't being ignored.

I've left explanatory feedback in the diff.

Lukasa · 2017-02-11T19:21:07Z

h2/settings.py

+            return NotImplemented
+
+    def __ne__(self, other):
+        return not self == other


Ok, this needs to be expanded a little bit. In particular, the if isinstance check needs to be repeated here so that we return NotImplemented if other is not a Settings object.

bhavishyagopesh · 2017-02-11T20:52:04Z

@Lukasa Sorry for being impatient(but you are certainly AWESOME =).) So what needs to be done next...

Lukasa · 2017-02-12T14:25:42Z

Ok, so I'd call this "good enough" to get going with. I think I'd also like us to add some more complex testing based on Hypothesis, which is really perfect for testing this kind of thing. Would you like to tackle that too, or would you like a break?

bhavishyagopesh · 2017-02-12T15:34:17Z

I think, I need to learn more about this project and python in general, so ,can this be merged first and I would love to continue doing whatever you suggest next but with a little more understanding of the project.

Lukasa · 2017-02-13T09:49:44Z

Ok cool, so we just need to wait until @alexwlchan is happy. =) @alexwlchan whenever you're happy, go ahead and merge.

alexwlchan · 2017-02-13T10:06:58Z

Personally, I’d prefer to see the commits squashed/rebased before merging. 66 is a few too many for my tastes.

(I’m happy to do the rebase and/or be overruled on this one.)

Lukasa · 2017-02-13T10:07:42Z

@alexwlchan Can you do a "rebase and merge" when you go ahead? We don't need the 66 commits.

alexwlchan · 2017-02-13T11:40:09Z

Superseded by #465.

Lukasa · 2017-02-13T11:42:24Z

Thanks so much @bhavishyagopesh! Per our one of the team policy, you'll find a GitHub invitation to the Hyper organisation in your inbox.

idealrealism added 2 commits February 9, 2017 20:51

Implemented Equality for settings object

ba33d81

Fixed self-generated indentation error

dcf9a07

Lukasa requested changes Feb 9, 2017

View reviewed changes

sethmlarson requested changes Feb 9, 2017

View reviewed changes

sethmlarson reviewed Feb 9, 2017

View reviewed changes

idealrealism added 4 commits February 10, 2017 02:15

Fixed style nits and also corrected _testsettings

5d56989

Update settings.py

a7cc38e

Update settings.py

dbda648

Update settings.py

8fb8355

sethmlarson previously approved these changes Feb 9, 2017

View reviewed changes

idealrealism added 3 commits February 10, 2017 03:01

Update test_settings.py

c79a101

Update test_settings.py

3e7eb7b

Update settings.py

bbb3ffa

alexwlchan requested changes Feb 9, 2017

View reviewed changes

idealrealism added 2 commits February 10, 2017 03:27

Update settings.py

ba3cb01

Update test_h2_upgrade.py

d81e0c9

alexwlchan reviewed Feb 10, 2017

View reviewed changes

Lukasa requested changes Feb 10, 2017

View reviewed changes

idealrealism added 10 commits February 10, 2017 17:43

Update test_h2_upgrade.py

da5915f

Update settings.py

d85e7ad

Update settings.py

dbed3a5

Addded Equality mixing class and also corrected some spacing issues

a7332d6

Update test_settings.py

df10df7

Equality mixin test and fixed style issues

9639952

Update test_settings.py

2fc7304

Equality mixin test and fixed style issues

0463579

Update test_h2_upgrade.py

52a3d68

Merge branch 'master' into master

1d4be39

idealrealism added 2 commits February 11, 2017 03:45

Update test_settings.py

10a9380

Update test_settings.py

b26bc3d

Lukasa requested changes Feb 11, 2017

View reviewed changes

Update test_settings.py

de2c6bc

Lukasa requested changes Feb 11, 2017

View reviewed changes

idealrealism added 2 commits February 12, 2017 02:03

Update settings.py

675f3b8

Update settings.py

1741a0e

Lukasa approved these changes Feb 13, 2017

View reviewed changes

alexwlchan approved these changes Feb 13, 2017

View reviewed changes

alexwlchan mentioned this pull request Feb 13, 2017

Rebased version of "Implementation of equality for the Settings class" #465

Merged

alexwlchan closed this Feb 13, 2017

This was referenced Feb 13, 2017

Implement equality for the Settings object. #453

Closed

Add support for property-based tests for Settings equality. #466

Open

Update test_h2_upgrade to use new Settings equality tools. #467

Closed



		def __eq__(self,other):
		return self._settings==other._settings

		@@ -254,12 +254,18 @@ def __len__(self):
		return len(self._settings)

		s[h2.settings.MAX_HEADER_LIST_SIZE]


		class test_settings_equality(object):

Conversation

bhavishyagopesh commented Feb 9, 2017 • edited by alexwlchan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethmlarson Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethmlarson commented Feb 9, 2017

Uh oh!

alexwlchan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhavishyagopesh commented Feb 10, 2017

Uh oh!

sethmlarson commented Feb 10, 2017

Uh oh!

bhavishyagopesh commented Feb 10, 2017

Uh oh!

bhavishyagopesh commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhavishyagopesh commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhavishyagopesh commented Feb 11, 2017

bhavishyagopesh commented Feb 9, 2017 •

edited by alexwlchan

Loading

sethmlarson Feb 10, 2017 •

edited

Loading

bhavishyagopesh commented Feb 11, 2017 •

edited

Loading

bhavishyagopesh commented Feb 11, 2017 •

edited

Loading

bhavishyagopesh commented Feb 11, 2017 •

edited

Loading

bhavishyagopesh commented Feb 12, 2017 •

edited

Loading