Merge lp:~rick-rickspencer3/lazr.restfulclient/add_gaurd_for_None_to_eq_functions into lp:lazr.restfulclient

[Rick Spencer]

I add a guard condition to return False in the _eq_ functions in the case where the object getting compared is None. This resolves the case where there is an exception due to NoneType not having expected members, see bug 495326.

I did not write any tests, unfortunately. The tests for docs were not very complete, and I couldn't figure out the api from it or how to use test fixtures.

[Rick Spencer (rick-rickspencer3)]

fix for bug 495326

[Leonard Richardson (leonardr)]

This is a good fix, but I won't land a branch without tests. I'm not sure what you mean by "the tests for docs were not very complete." My best guess is that you found the test fixtures confusing and couldn't locate the doctests.

An example should help you see how the test system works. To test your two changes I added tests to src/lazr/restfulclient/docs/entries.txt (section "Comparing entries") and src/lazr/restfulclient/docs/hosted-files.txt (section "Comparing hosted files"). My diff is below.

Since this branch now contains my code I'm going to get someone else to review it, and then I'll land it.

=== modified file 'src/lazr/restfulclient/NEWS.txt'
--- src/lazr/restfulclient/NEWS.txt 2009-12-15 12:28:41 +0000
+++ src/lazr/restfulclient/NEWS.txt 2009-12-17 14:55:45 +0000
@@ -5,6 +5,7 @@
 Development (XXXX-XX-XX)
 ========================
 - Added a proof-of-concept test for OAuth-signed anonymous access.
+- Fixed comparisons of entries and hosted files with None.

 0.9.10 (2009-10-23)
 ===================

=== modified file 'src/lazr/restfulclient/docs/entries.txt'
--- src/lazr/restfulclient/docs/entries.txt 2009-10-21 12:12:44 +0000
+++ src/lazr/restfulclient/docs/entries.txt 2009-12-17 14:41:24 +0000
@@ -402,6 +402,11 @@
     >>> recipe == another_recipe
     False

+An entry is never equal to None.
+
+ >>> recipe == None
+ False
+
 If one entry represents the current state of the server, and the other
 is out of date or has client-side modifications, they will not be
 considered equal.

=== modified file 'src/lazr/restfulclient/docs/hosted-files.txt'
--- src/lazr/restfulclient/docs/hosted-files.txt 2009-10-21 12:12:44 +0000
+++ src/lazr/restfulclient/docs/hosted-files.txt 2009-12-17 14:51:41 +0000
@@ -89,7 +89,6 @@
 Two hosted file objects are the same if they point to the same
 server-side resource.

-
     >>> cover = service.cookbooks['Everyday Greens'].cover
     >>> cover_2 = service.cookbooks['Everyday Greens'].cover
     >>> cover == cover_2
@@ -99,6 +98,11 @@
     >>> cover == other_cover
     False

+A hosted file is never equal to None.
+
+ >>> cover == None
+ False
+
 Error handling
 --------------

=== modified file 'src/lazr/restfulclient/resource.py'
--- src/lazr/restfulclient/resource.py 2009-10-23 13:22:44 +0000
+++ src/lazr/restfulclient/resource.py 2009-12-17 14:43:27 +0000
@@ -365,6 +365,8 @@
         retrieve or modify the hosted file contents is to open a
         filehandle, which goes direct to the server.
         """
+ if other is None:
+ return False
         return self._wadl_resource.url == other._wadl_resource.url

@@ -600,6 +602,7 @@
         contain the same values.
         """
         return (
+ other is not None and
             self.self_link == other.self_link and
             self.http_etag == other.http_etag and
             self._dirty_attributes == other._dirty_attributes)

[Michael Nelson (michael.nelson)]

Looks great guys!

At first I was confused about the purpose of the documentation:

> +An entry is never equal to None.
> +
> + >>> recipe == None
> + False

as recipe is obviously not None in the documentation, but then realised this is really showing that we don't fall over when comparing with None right? In which case, it probably belongs in a unit test in src/lazr/restfulclient/tests (or explicitly say here 'An entry can be compared with None without blowing up' ;), but not sure how useful that is as documentation)? But I'll leave that up to you Leonard.

Thanks!

review: Approve (code)

[Leonard Richardson (leonardr)]

Michael, good catch. I've reworded the doctest to make clear what's being tested and I will land the branch. I don't think a unit test works here because lazr.restfulclient uses doctests for everything that simulates usage of the client.

=== modified file 'src/lazr/restfulclient/resource.py'
--- src/lazr/restfulclient/resource.py	2009-10-23 13:22:44 +0000
+++ src/lazr/restfulclient/resource.py	2009-12-11 17:07:12 +0000
@@ -365,6 +365,12 @@
         retrieve or modify the hosted file contents is to open a
         filehandle, which goes direct to the server.
         """
+
+        #gaurd condition to ensure that other is not None
+        #by definition, they are not equal if other is None
+        if other is None:
+            return False
+
         return self._wadl_resource.url == other._wadl_resource.url
 
 
@@ -599,6 +605,11 @@
         attributes are the same, and if their dirty attribute dicts
         contain the same values.
         """
+        #gaurd condition to ensure that other is not None
+        #by definition, they are not equal if other is None
+        if other is None:
+            return False
+
         return (
             self.self_link == other.self_link and
             self.http_etag == other.http_etag and