Exception not requests.Response with BaseNomadException str dunder#109
Exception not requests.Response with BaseNomadException str dunder#109erhlee-bird merged 3 commits intomasterfrom
Conversation
…onse object and has text attribute. If not stringify given exception/value.
erhlee-bird
left a comment
erhlee-bird
left a comment
There was a problem hiding this comment.
I have the same reactions to you about the Exception substring tests and conditional __str__, but it looks very reasonable to me.
|
|
||
| try: | ||
| n = nomad.Nomad( | ||
| host="162.16.10.102", |
There was a problem hiding this comment.
Out of curiosity, is this an actual host of importance or just a dummy value?
There was a problem hiding this comment.
Completely arbitrary dummy value!
|
|
||
| _ = n.job.deregister_job("example") | ||
|
|
||
| except nomad.api.exceptions.BaseNomadException as err: |
There was a problem hiding this comment.
assertRaises as a context manager ends up being a clearer way of handling this type of test case imho.
Edit: beyond that, it's more effective and guarding against unintended changes in behavior. That is, this test case ensures we get the behavior we expect when an exception is raised. However if an exception isn't raised at all (for whatever reason) it will still pass, no?
There was a problem hiding this comment.
Completely agree with your points, I don't know why I decided not to use pytest.raises context manager, since ive used it elsewhere throughout the tests corpus.. must have been the "lateness of the night" :| , I'll swap that out!
erhlee-bird
left a comment
erhlee-bird
left a comment
There was a problem hiding this comment.
I like the change made as suggested by @jeffwecan with the context manager for the exception.
|
|
||
| def __str__(self): | ||
| return 'The {0} was raised with following response: {1}.'.format(self.__class__.__name__, self.nomad_resp.text) | ||
| if isinstance(self.nomad_resp, requests.Response) and hasattr(self.nomad_resp, "text"): |
There was a problem hiding this comment.
It does seem rather funky to have this logic be part of the exception itself. I am tempted to suggest it would be better to tackle things from the other direction. For example something like:
- Have
BaseNomadExceptionjust be a stock pythonException - Add a
NomadRequestExceptionclass that can include this response.text printing logic - Update https://github.com/cloudbuy/python-nomad/blob/e10abe4191d96a0715c29eb00bbc2f7051af2d15/nomad/api/base.py#L159 to use
NomadRequestException - Leave https://github.com/cloudbuy/python-nomad/blob/e10abe4191d96a0715c29eb00bbc2f7051af2d15/nomad/api/base.py#L162 as-is
However I also feel like that sort of change introduces its own fiddly bits for various reasons so I can live with this route :)
3 participants
Addressing #107, add safety checks to verify that the passed parameter of
nomad_respis arequests.Responseobject if it is not cast str around exception with different message.Not a huge fan of have a conditional str with different messages (along with checking for certain substrings as a test) but this should at least fix the bug and ensure somewhat backwards compatibility. Technically
textin requests.Response is not a guarantee either since its kwargs are popped for response and request and can possibly beNone.If we do eventually move to python 3+ only support adding annotations should help a bunch when visually inspecting or even mypy level runtime checking.