Partially initialized Items cannot be repr()'d

Not ship critical.

Here's the simple patch.

--- attributes.py       (revision 12038)
+++ attributes.py       (working copy)
@@ -420,7 +420,8 @@
             return self

         pyval = getattr(oself, self.underlying, _NEEDS_FETCH)
-        st = getattr(oself, 'store')
+                               #  st was not used anywhere anyway
+                               #  st = getattr(oself, 'store')
         if pyval is _NEEDS_FETCH:
             dbval = getattr(oself, self.dbunderlying, _NEEDS_FETCH)
             if dbval is _NEEDS_FETCH:

UnitTest :

Index: test_item.py
===================================================================
--- test_item.py        (revision 12038)
+++ test_item.py        (working copy)
@@ -302,6 +302,17 @@
                           1234)
         self.assertEquals(st.getItemByID(oldStoreID, default=1234),
                           1234)
+    def testItemWithNoStoreReps(self):
+       """
+          Bug #993, Item with no store defined would break on repr
+       """
+       class Y(item.Item):
+         y = text()
+       from epsilon import spewer
+       s = spewer.Spewer()
+       s.toggle()
+       Y()
+       s.toggle()

Thanks for the patch!

Review comments:

• I think the test belongs in test_reprs.
• new tests are named as test_xxxYyy, not testXxxYyy.
• invoking the spewer here seems like overkill. Isn't there a simpler way to invoke the repr at the appropriate time?
• The test should be asserting something, not simply running some code and hoping that no exception is raised.
• The indentation on the test docstring is off. The docstring should be indented to the same place that the quotes on the line above are.
• the deleted line should just be deleted, not commented out. It doesn't help anyone reading the code later.
• The test docstring should explain what it's verifying, not describing the bug that existed before the test was written. We don't generally link to the bug number, although perhaps that is useful information.

I think I addressed all comments. I didn't know what to test for so, I tested that repr returns a string.

Index: test/test_reprs.py
===================================================================
--- test/test_reprs.py  (revision 12038)
+++ test/test_reprs.py  (working copy)
@@ -61,3 +61,12 @@
                             ReprTesterItemClass.txtattr.descending)))
         self.assertIn(repr(ReprTesterItemClass.intattr.ascending), R)
         self.assertIn(repr(ReprTesterItemClass.txtattr.descending), R)
+
+    def test_itemWithNoStoreReps(self):
+       """
+       Verify, that items that are instantiated without a C{axiom.store.Store) 
+       __repr__ correctly
+       """
+       class Y(Item):
+         y = text()
+       self.assertEquals(type(Y().__repr__()),str)
Index: attributes.py
===================================================================
--- attributes.py       (revision 12038)
+++ attributes.py       (working copy)
@@ -420,7 +420,6 @@
             return self

         pyval = getattr(oself, self.underlying, _NEEDS_FETCH)
-        st = getattr(oself, 'store')
         if pyval is _NEEDS_FETCH:
             dbval = getattr(oself, self.dbunderlying, _NEEDS_FETCH)
             if dbval is _NEEDS_FETCH:

I attached a better test case and petition for closure of this ticket

Still some problems. The biggest one is:

• The test passes in trunk without the non-test part of the patch applied, so it doesn't test anything useful.

Smaller issues while you are fixing that though:

• The use of regular expressions is superfluous and misleading. startswith would work better.
• "reps" is slang for repetitions in exercise. Perhaps you meant to end the test's name with "reprs".
• There's trailing whitespace in the new test. If you use emacs, check out the "show-trailing-whitespace" option and the "trailing-whitespace" face. If you use vim, have a look at the "listchars" option.

My emacs config:

(custom-set-variables ... '(show-trailing-whitespace t) ... )

My vim config:

set list
set listchars=tab:>-,trail:·

I can't repeat the error without epsilon.spewer active. So, I am inclined to put the spewer back in the tests.

Wow. Item.__repr__ is an abomination. Now that I see the full scope of this, I can't help but try to fix it.

Tested and fixed in item-repr-993

Haha. Looks good, though a docstring for test_item.ItemTestCase would be nice.

(In [13809]) Merge item-repr-993

Author: exarkun Reviewer: moea Fixes #993

Add tests for several cases of Item.repr, remove the special handling of exceptions raised by attribute.reprFor, and remove the unused item.store lookup in SQLAttribute.get.