SETTING_DELETED isn't working as intended

[jazzband]

SETTING_DELETED isn't working as intended

[jcassee]

Hi Eric,

Good catch! Great to see you added tests to verify your fix. I will pull your changes and release a new version.

Regards,
Joost

[jcassee]

I merged your branch, but decided not to release a new version for this fix. It was not a user-visible bug, so let's not bother them just yet.

Thanks again!

• Joost

[edavis]

Sounds good.

[edavis]

Strange. I just tested my setting-deleted branch on linux and now I'm getting "maximum recursion depth errors." Hold off on cutting a release until I can figure this out. You didn't get any errors, right?

It's always something :)

[edavis]

edavis/django-analytical@master...setting-deleted

As best I can tell, copy.copy was trying to getattr self.default_settings until it hit the maximum recursion depth. This fix overrides __getattr__ so self.default_settings gets returned as normal.

I have a suspicion it's a Python 2.6 vs. 2.7 thing but won't be able to test it until later.

But anyway, you can find two commits in my setting-deleted branch. Let me know if you catch anything else.

UPDATE: I can confirm it was a 2.6 vs. 2.7 thing. Testing against both versions -- with tox -- shows everything in setting-deleted passing as intended.

[jcassee]

Hi Eric,

Thanks for the heads-up. I did not encounter this problem, but I have
never felt really comfortable using a system 'copy' function (maybe a
hesitation left over from my Java days). The new code looks good in
any case.

There is just one problem with the test code when I run "python2.6
setup.py test": the structure "with self.assertRaises" does not work
on Python 2.6. (From the docs: "Changed in version 2.7: Added the
ability to use assertRaises() as a context manager.")

I now also understand why my original code did not work: a class
descriptor is set on a class, not an instance.

Regards,
Joost

2011/9/17 Eric Davis
[email protected]:

edavis/django-analytical@master...setting-deleted

As best I can tell, copy.copy was trying to getattr self.default_settings until it hit the maximum recursion depth.  This fix short-circuits it so default_settings gets returned.

I have a suspicion it's a Python 2.6 vs. 2.7 thing but won't be able to test it until later.

But anyway, you can find two commits in my setting-deleted branch.  Let me know if you catch anything else.

Reply to this email directly or view it on GitHub:
#8 (comment)

Joost Cassee
http://joost.cassee.net

[edavis]

Hi Joost,

Yeah, I'm not a huge fan of using copy like that but it's the only way I've found to create a completely isolated instance of settings._wrapped which is needed to "add back" deleted tests.

Good catch on the assertRaises context manager problem. I've changed all assertRaises context managers into "regular" method calls. Turns out you can use it as a context manager with Python 2.6 and Django 1.3 because Django 1.3 bundles unittest2 which includes it as a context manager. But it does fail with Python 2.6 and Django 1.2.

To be on the safe side, I also check for assertRaisesRegexp and use it if it exists, but use plain assertRaises if it doesn't.

To help manage all of these versions, I also added a tox.ini config file for use with tox. With it I test against:

• Python 2.6 and Django 1.2
• Python 2.7 and Django 1.2
• Python 2.6 and Django 1.3
• Python 2.7 and Django 1.3

So if you could pull from edavis/django-analytical@master...setting-deleted again and test it out, I think this should do it.

[jcassee]

Hi Eric,

Using tox is a good idea. I don't understand one thing, though: why not just use the Python 2.6 way of doing assertRaises? That also works on 2.7, right?

Regards,
Joost

[edavis]

I assume you mean in the test_get_required_setting method? If so, I wanted to add a regression test to make sure get_required_setting fails on getattr rather than the regexp check.

So if they have assertRaisesRegexp I use it but if they don't I fallback to a generic assertRaises.

Though, now that you mention it, if they're using Python 2.6 it won't be much of a regression test because AnalyticalException gets raised for both missing settings and regexp check failures... kind of defeating the purpose.

Maybe subclass AnalyticalException into AnalyticalMissingSetting and AnalyticalIncorrectSettingRegex and use assertRaises against that instead of checking the exception message. Could be overkill, though.

What do you think?

[jcassee]

Hi Eric,

Yes, I meant specifically these lines:
# only available in python >= 2.7
if hasattr(self, 'assertRaisesRegexp'):
with self.assertRaisesRegexp(AnalyticalException, "^USER_ID
setting: not found$"):
user_id = get_required_setting("USER_ID", "\d+", "invalid USER_ID")
else:
self.assertRaises(AnalyticalException,
get_required_setting, "USER_ID", "\d+",
"invalid USER_ID")

I understand what you are doing now. I think it is fine as it is.
Subclassing AnalyticalException feels like overkill at the moment, and
I will be testing django-analytical on Python 2.7 anyway. The
conditional is just to make it pass on Python 2.6.

Looks like this issue can be closed for real, now, don't you think?

Regards,
Joost

2011/9/18 Eric Davis
[email protected]:

I assume you mean in the test_get_required_setting method?  If so, I wanted to add a regression test to make sure get_required_setting fails on getattr rather than the regexp check.

So if they have assertRaisesRegexp I use it but if they don't I fallback to a generic assertRaises.

Though, now that you mention it, if they're using Python 2.6 it won't be much of a regression test because AnalyticalException gets raised for both missing settings and regexp check failures... kind of defeating the purpose.

Maybe subclass AnalyticalException into AnalyticalMissingSetting and AnalyticalIncorrectSettingRegex and use assertRaises against that instead of checking the exception message.  Could be overkill, though.

What do you think?

Reply to this email directly or view it on GitHub:
#8 (comment)

Joost Cassee
http://joost.cassee.net

[edavis]

Yeah, I think this should about do it. Thanks for your help and feedback.

[jcassee]

Hah! No, thank you for contributing! :-)