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

Confusing use of URLNotFoundNomadException #59

Open
jeteon opened this issue Aug 18, 2018 · 2 comments
Open

Confusing use of URLNotFoundNomadException #59

jeteon opened this issue Aug 18, 2018 · 2 comments

Comments

@jeteon
Copy link
Contributor

jeteon commented Aug 18, 2018

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:

def post(self, endpoint, params=None, data=None, json=None, headers=None):
url = self._urlBuilder(endpoint)
if self.token:
try:
headers["X-Nomad-Token"] = self.token
except TypeError:
headers = { "X-Nomad-Token": self.token }
response = None
try:
response = self.session.post(url,
params=params,
json=json,
headers=headers,
data=data,
verify=self.verify,
cert=self.cert,
timeout=self.timeout)
if response.ok:
return response
if response.status_code == 400 or response.status_code == 404 or response.status_code == 500:
raise nomad.api.exceptions.URLNotFoundNomadException(response)
except requests.RequestException:
raise nomad.api.exceptions.BaseNomadException(response)

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
Copy link
Owner

jrxFive commented Aug 19, 2018

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
Copy link
Contributor Author

jeteon commented Sep 13, 2018

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.

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