atlassian-api / atlassian-python-api

Atlassian Python REST API wrapper
https://atlassian-python-api.readthedocs.io
Apache License 2.0
1.34k stars 661 forks source link

the API should not log exceptions and return "None". #1229

Open jmatzdorff-cpi opened 1 year ago

jmatzdorff-cpi commented 1 year ago

When searching for a page (using get_page_by_title) if the page does not exist, this currently gets executed:

except (IndexError, TypeError) as e:
     log.error("Can't find '%s' page on the %s!", title, self.url)
     log.debug(e)
     return None

(it also happens in update_page).

Please don't do this -- but rather return the exception back to the caller to handle what to do with the exception. The caller may want to handle it in some way and not have a ton of debug output printed out to the console or what not.

merely removing the try/except block would be much more useful.

Spacetown commented 1 year ago

But that's the way Python works. When there is a file open error you get also an exception which can be caught. Returning None has to be handled in each level of the calling stack which makes code harder to maintain.

jmatzdorff-cpi commented 1 year ago

You are correct which is why the way it's done now (as I stated above) isn't good. Currently it gobbles up the Index/Type errors, prints a message out to the log (both error and debug) and returns None. which means i would need to do something like if a.get_page_by_title(<parms>) is not None anytime i want to see if a page is found or not. and if the page is not found, a ton of stuff is printed to the console.

I'd rather do

try:
   a.get_page_by_title(<parms>)
except <whateverExceptionsIWantToDealWith>:
   deal_with_these_exceptions

and now, it either does an exception the bubbles all the way up to termination, or i get to decide what to do with the exception.

right now if and Index or Type error happens, I have no access to that exception information when I call the API, which is bad design.

Spacetown commented 1 year ago

Ok. I understood it the other way. In my opinion an exception would be better here. The problem is that this change is incompatible with the current behavior.

gonchik commented 1 year ago

@jmatzdorff-cpi in which way do you think better to wrap it ?

Spacetown commented 1 year ago

@gonchik I think the more Pythonic way is not to catch the exception and return None. Let the user catch the exceptions.

gonchik commented 1 year ago

@Spacetown how about catching general exception writing into log and then raising with that ? :) like


     log.error("Can't find '%s' page on the %s! - with %e", title, self.url, e)
     raise e
     return None```
Spacetown commented 1 year ago

But why logging it? This can the caller do if he want to have it. Removing the try/catch is enough.

gonchik commented 1 year ago

okay ;)