Merge lp:~jtatum/mago/pidgin-exception into lp:~mago-contributors/mago/mago-1.0

Proposed by James Tatum on 2009-08-26
Status: Merged
Merged at revision: not available
Proposed branch: lp:~jtatum/mago/pidgin-exception
Merge into: lp:~mago-contributors/mago/mago-1.0
Diff against target: None lines
To merge this branch: bzr merge lp:~jtatum/mago/pidgin-exception
Reviewer Review Type Date Requested Status
Ara Pulido 2009-08-31 Approve on 2009-09-07
Javier Collado (community) Needs Information on 2009-08-28
Nagappan Alagappan 2009-08-26 Approve on 2009-08-27
Review via email: mp+10774@code.launchpad.net
To post a comment you must log in.
James Tatum (jtatum) wrote :

If the pidgin suite is invoked and python-pyxmpp or python-msn are not installed, mago crashes. This handles the imports so import errors throw exceptions at test execution time rather than at import time.

Nagappan Alagappan (nagappan) wrote :

I think, you can use

if ClientXMPP:
    self.client = ClientXMPP(userid, passwd)
else:
    raise Exception, "pidgin testing requires python-pyxmpp package."

The same can be applied to ClientMSN

review: Approve
Javier Collado (javier.collado) wrote :

Hello,

I'm not sure I agree on this change. I would say that the correct way to solve this issue is at the package level, that is, adding those packages as dependencies to mago.

In my opinion, catching ImportError exception should be done if there's an alternative library that could do the same job (for example some xml libraries implement the same ElementTree interface) so the problem can be solved at runtime. If the problem isn't really solved, then the exception should be raised as soon as possible to be able to locate the problem and fix it.

Please let me know what do you think about this.

Best regards,
    Javier

review: Needs Information
James Tatum (jtatum) wrote :

Hi Javier,

I agree in principle. Many mago tests already require some configuration of some kind or another and so users should be examining the various README files to see what the prerequisites are. I think the case here is for users that want to use the mago framework but who aren't running all the test suites that are shipping with it. Should the mago package pull in a bunch of libraries that only get used for specific tests? This is a philosophical question.

If the answer is yes, we should probably add the debian directory in to the bzr tree so dependencies can get added as they are added to the source. Mago is not just an ubuntu project, so they need to be documented elsewhere as well.

For what it's worth, without this patch, mago actually crashes when running the suite and does not output a log for the pidgin tests or continue to more tests if you do not have one of the libraries.

Rick McBride (rmcbride) wrote :

Hi,

I think for these particular imports that this conditional handling is appropriate. I'd like to be able to run these tests without adding packages that I may not otherwise have installed. The success/failure of a given pidgin installations should not be dependent on me having the MSN or XMPP modules available.

> Hello,
>
> I'm not sure I agree on this change. I would say that the correct way to solve
> this issue is at the package level, that is, adding those packages as
> dependencies to mago.
>
> In my opinion, catching ImportError exception should be done if there's an
> alternative library that could do the same job (for example some xml libraries
> implement the same ElementTree interface) so the problem can be solved at
> runtime. If the problem isn't really solved, then the exception should be
> raised as soon as possible to be able to locate the problem and fix it.
>
> Please let me know what do you think about this.
>
> Best regards,
> Javier

Ara Pulido (ara) wrote :

I agree with the change. If included in the packaging it should only be included as a recommends.

If the user does not have it installed, then putting a correct error is better

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'mago/application/pidgin/buddy.py'
2--- mago/application/pidgin/buddy.py 2009-06-03 14:22:04 +0000
3+++ mago/application/pidgin/buddy.py 2009-08-17 16:11:02 +0000
4@@ -2,8 +2,15 @@
5
6 import gobject, gtk
7
8-from xmpp_utils import ClientXMPP
9-from msn_utils import ClientMSN
10+try:
11+ from xmpp_utils import ClientXMPP
12+except ImportError:
13+ ClientXMPP = None
14+
15+try:
16+ from msn_utils import ClientMSN
17+except ImportError:
18+ ClientMSN = None
19
20 class _Buddy(object):
21 """
22@@ -72,7 +79,10 @@
23 def __init__(self, userid, passwd):
24
25 self.account = (userid, passwd)
26- self.client = ClientXMPP(userid, passwd)
27+ try:
28+ self.client = ClientXMPP(userid, passwd)
29+ except AttributeError:
30+ raise Exception, "pidgin testing requires python-pyxmpp package."
31
32 def connect(self, register=False, name='', email=''):
33 def _idle_cb(client):
34@@ -138,7 +148,10 @@
35 class _BuddyMSN(_Buddy):
36 def __init__(self, userid, passwd):
37 self.account = (userid, passwd)
38- self.client = ClientMSN(self.account)
39+ try:
40+ self.client = ClientMSN(self.account)
41+ except AttributeError:
42+ raise Exception, "pidgin testing requires python-msn package."
43
44 def connect(self, register=False, name='', email=''):
45 def _idle_cb(client):

Subscribers

People subscribed via source and target branches

to status/vote changes: