search

jrxFive / python-nomad

Client library Hashicorp Nomad
https://python-nomad.readthedocs.io/en/latest/
MIT License
139 stars 73 forks source link

Confusing use of URLNotFoundNomadException #59

Open jeteon opened 6 years ago

jeteon commented 6 years ago

I was working with the project when I encountered an issue with my inputs that manifested itself as an uncaught URLNotFoundNomadException. It took me a while to figure out that this did not actually refer to an invalid URL being used but rather is a blanket exception for almost anything that can go wrong with a request to Nomad as I eventually saw from here on lines 121-122:

https://github.com/jrxFive/python-nomad/blob/90e94ba781c3af32e3f2ec14d279d92dc96711c5/nomad/api/base.py#L100-L125

I think it is rather confusing to use this exception for responses that have anything other than a 404 status code. Would it not be more appropriate to use the BaseNomadException class for the general case of something going wrong?

jrxFive commented 6 years ago

Hey @jeteon, sorry for the confusion ( I do agree it is confusing ). We could add BaseNomadException as the parent class/inherited class for UrlNotFoundNomadException so you would be able to just handle that. I should have done that from the get go. There are some cases for example if you were to look up a job that doesn't exist Nomad would return a 404 status code back raising UrlNotFoundNomadException. Let me know what you think about the inheritance it should be a non-breaking change.

jeteon commented 6 years ago

The inheritance should be non-breaking and would work yes. I guess I was coming from the perspective of discovering the library interactively so the displayed exception is what matters and not so much what I would be trying to catch in code. Perhaps if there was a child of UrlNotFoundNomadException then existing implementations could continue to catch that but a more semantically correct exception would be raised and used in future implementations.