CCI-MOC / hil

Hardware Isolation Layer, formerly Hardware as a Service
Apache License 2.0
24 stars 54 forks source link

Improve JSON-less 404 error readability and prevention with client library #926

Closed ianballou closed 6 years ago

ianballou commented 6 years ago

This code aims to stop the Unexpected error: No JSON object could be decoded error mentioned in #910:

Note: some commands, such as switch_register, have not been implemented in the client library yet. These will get checks for reserved characters when they are implemented.

ianballou commented 6 years ago

Just wanted to note that I'm back working for the new semester so I'll have activity on here soon.

ianballou commented 6 years ago

My latest commit cleans up the methods for checking for reserved characters.

ianballou commented 6 years ago

The tests for this are in progress now.

ianballou commented 6 years ago

I've setup decorators/wrappers to check the illegal characters now, so the client functions should remain otherwise unedited.

ianballou commented 6 years ago

I've addressed the requests and added a first pass at the tests. Did I address your requests correctly @zenhack and @naved001 ?

naved001 commented 6 years ago

I don't have any other comments to make on this at this point, @zenhack has already made some good points.

naved001 commented 6 years ago

actually, don't worry about it.

On Tue, Feb 6, 2018 at 3:28 PM, Ian Ballou notifications@github.com wrote:

@Izhmash commented on this pull request.

In hil/client/base.py https://github.com/CCI-MOC/hil/pull/926#discussion_r166432879:

         e = json.loads(response.content)

raise FailedAPICallException( error_type=e['type'], message=e['msg'], )

  • Catching responses that do not return JSON

  • except ValueError:
  • return response.content
  • +def _find_reserved(string, slashes_ok=False):

  • """Returns a list of illegal characters in a string"""
  • if slashes_ok:
  • p = r"[^A-Za-z0-9 /$_.+!*'(),-]+"
  • else:
  • p = r"[^A-Za-z0-9 $_.+!*'(),-]+"
  • return list(x for l in re.findall(p, string) for x in l)

I might be wrong, but I think this is one of those cases where the list comprehension is actually less complex than if I didn't use a one-liner. I can add a comment for clarification if that helps.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CCI-MOC/hil/pull/926#discussion_r166432879, or mute the thread https://github.com/notifications/unsubscribe-auth/ATLmqB-yO1cFIPjvQmgM3ubT6papKBBCks5tSLXugaJpZM4RD9GS .

ianballou commented 6 years ago

@zenhack I believe I've addressed all of your requests in these two commits

zenhack commented 6 years ago

LGTM