Pull requests

#2 Merged
Repository
tbaxx tbaxx
Branch
default
Repository
jgoettsch jgoettsch
Branch
default

Removed confusing exception handling; let the exception propagate as exception, not as a return value.

Author
  1. Theodoros Balopoulos
Reviewers
Description
No description

Comments (3)

  1. Jeffrey Goettsch repo owner

    This exposes a logic flaw I hadn't noticed.

    I intended to shield the user from having to know or care about any exceptions that Requests could raise. Of course, this code doesn't do that. If you force a connection error, for example, using the NMA client, it simply causes another exception when trying to parse the result:

    Traceback (most recent call last):
      File "runner.py", line 29, in <module>
        main()
      File "runner.py", line 19, in main
        result = client.notify('message', 'test event', split=True)
      File "/home/jeff/Workspace/py-pushnotify/py-pushnotify/pushnotify/nma.py", line 182, in notify
        send_notify(this_desc, event, kwargs)
      File "/home/jeff/Workspace/py-pushnotify/py-pushnotify/pushnotify/nma.py", line 172, in send_notify
        self._parse_response(response)
      File "/home/jeff/Workspace/py-pushnotify/py-pushnotify/pushnotify/nma.py", line 81, in _parse_response
        xmlresp = response.text
    AttributeError: 'ConnectionError' object has no attribute 'text'
    

    So now we have to catch a different exception, which is less desirable than your patch.

    Given the exceptions that Requests raises, I don't think my original intent can be accomplished. However, I'm going to think on it for a few days before deciding to approve this or not.

  2. Theodoros Balopoulos author

    Thanks for your input.

    I think the requests module has a very clear exception hierarchy. There are few exceptions, each one with a clear scope and they all have a common father. So not sure if there is any benefit isolating the user from them.

    The AttributeError exception is very misleading; getting a ConnectionError when there is a connection error is so much better! I have been using pushnotify with my small change in one of my projects for a few weeks without any issues.