Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throwing generic exceptions makes handling them differently impossible #43

Closed
stefanct opened this issue Jul 10, 2021 · 5 comments
Closed

Comments

@stefanct
Copy link

The library often handles exception itself, e.g., cacheObject's __query(), by excepting, logging, and then raising a newly created generic Exception object. For example very specific exceptions raised by urllib get converted into this generic type. It thus becomes impossible to differentiate between different errors on the user side, e.g., it one cannot even react differently if the user hits ctrl+c while a transfer is in progress.
As is it is completely impossible to do meaningful error handling without inspection or other tricks AFAICT.

It would be much better to leave the respective error handling to the library user instead. You can still add useful information to the exception (and log stuff if you like), cf. https://wiki.python.org/moin/HandlingExceptions

@stefanct stefanct changed the title Encapsulating exceptions makes handling them differently impossible Throwing generic exceptions makes handling them differently impossible Jul 11, 2021
@mocnik-science
Copy link
Owner

Dear @stefanct,

This is a good point. The motivation to throw more generic types of exceptions is that many people use OSMPythonTools to write scripts rather than larger pieces of software. The generic types of exceptions raised should be more suitable to easily understand where an issue is, and python should usually show the original exceptions in the command line anyway.

Still, I understand why it would be great to have access to the original exceptions, which is why I have added these as a second argument to the Exception raised. I hope that this helps.

Best

@stefanct
Copy link
Author

In my case that idea didn't really work out TBH :) It took me a lot more time to figure out what is happening and why, e.g, my ctrl+c code does not work, why I cannot catch the http errors etc. Even more so because - as you say - the library logs the underlying types... but you don't receive that but the generic one eventually. That's very surprising. I had to actually look at the source code of the library to figure it out. I was also using this just for a "quick" hack/one-time executed script but implementing the error handling took the majority of the time because of what you describe as a feature :)
Take a look at the ugly string parsing I have to do to change the behavior... and that's not because I wanted to do some fancy feature or implement it very elegantly or something but because I have to distinguish timeouts from too-many-requests from ctrl+c from actual/other errors:
https://github.com/stefanct/osm_refhistorymeta/blob/main/ref_contributors.py#L80
It would already be a help if you wouldn't add the string representation of the original exception (only) but the exception instance itself.

That's just my feedback/2ct - I am fine. Thanks for your work!

PS: The printing of the exceptions themselves is also something I would strongly recommend against in general in a library. That's not the right place but I get your rationale regarding that in this case (I still wouldn't have done it though :)

@mocnik-science
Copy link
Owner

Dear @stefanct,

Did you check the newest version? As I wrote before, I changed the behaviour before providing my answer. The original exceptions should be forwarded as part of the generic ones, not just the titles. (see 6d37ce7)

Printing the exceptions here has one very simple reason. I print also usual logs related to the download process with a simple reason in mind. The user should be aware of the fact that data is downloaded. There have been issues with querying to much data and thus overusing servers, which is why being aware of server usage is highly important. When usual logs are printed, the execution should IMHO be too.

Best

@stefanct
Copy link
Author

Ah I see. I didn't get that because the string was also added before as the second argument :) Will take a look at it if need be... thanks!

@mocnik-science
Copy link
Owner

Feel welcome to have a look. ;) I appreciate feedback, as you can see here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants