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

added type hinting, document string, TODO statements, fixed small typ… #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ This is an open-source client library communicating with the Mujin Controller We

- Versions have three numbers: MAJOR.MINOR.PATCH

- All versions with the same MAJOR.MINOR number have the same API ande are ABI compatible.
- All versions with the same MAJOR.MINOR number have the same API and are ABI compatible.


## Running on Linux
Expand Down
25 changes: 14 additions & 11 deletions python/mujinwebstackclient/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from .version import __version__ # noqa: F401

import six
from typing import Optional

try:
import ujson as json # noqa: F401
Expand Down Expand Up @@ -49,15 +50,15 @@ class ClientExceptionBase(Exception):
"""
_message = None # the error message, should be unicode

def __init__(self, message=''):
if message is not None and not isinstance(message, six.text_type):
def __init__(self, message: Optional[str]='') -> None:
if message is not None and not isinstance(message, six.text_type):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the added space?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The space is indeed not needed.

message = message.decode('utf-8', 'ignore')
self._message = message

def __str__(self):
def __str__(self) -> str:
return u'%s: %s' % (self.__class__.__name__, self._message)

def __repr__(self):
def __repr__(self) -> str:
return '<%s(message=%r)>' % (self.__class__.__name__, self._message)


Expand All @@ -69,7 +70,7 @@ class APIServerError(ClientExceptionBase):
_detailInfo = None # dcit, the detailed info
_inputcommand = None # the command sent to the server

def __init__(self, message, errorcode=None, inputcommand=None, detailInfoType=None, detailInfo=None):
def __init__(self, message: str, errorcode: Optional[int]=None, inputcommand: Optional[str]=None, detailInfoType: Optional[str]=None, detailInfo: Optional[dict]=None) -> None:
if message is not None and not isinstance(message, six.text_type):
message = message.decode('utf-8', 'ignore')
self._message = message
Expand All @@ -78,7 +79,7 @@ def __init__(self, message, errorcode=None, inputcommand=None, detailInfoType=No
self._detailInfoType = detailInfoType
self._detailInfo = detailInfo

def __str__(self):
def __str__(self) -> str:
if self._message is not None:
return _('API Server Error: %s')%self._message

Expand All @@ -88,7 +89,7 @@ def __repr__(self):
return '<%s(message=%r, errorcode=%r, inputcommand=%r, detailInfoType=%r, detailInfo=%r)>' % (self.__class__.__name__, self._message, self._errorcode, self._inputcommand, self._detailInfoType, self._detailInfo)

@property
def message(self):
def message(self) -> str:
"""The error message from server."""
return self._message

Expand All @@ -98,16 +99,17 @@ def errorcode(self):
return self._errorcode

@property
def stacktrace(self):
def stacktrace(self) -> str:
"""The stack trace for the error"""
return ''

@property
def inputcommand(self):
def inputcommand(self) -> Optional[str]:
"""The command that was sent to the server. Could be None."""
return self._inputcommand

@property
def detailInfoType(self):
def detailInfoType(self) -> Optional[str]:
"""string for the detai info type"""
return self._detailInfoType

Expand All @@ -125,7 +127,7 @@ class AuthenticationError(ClientExceptionBase):


class WebstackClientError(ClientExceptionBase):

"""Exeption class for errors returned by the web stack client"""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wording makes me a bit uncomfortable since there's a few error types that a request could raise:

python/mujinwebstackclient/controllerwebclientraw.py
115:            raise WebstackClientError(_('Timeout value (%s sec) is too small') % timeout)
180:                raise APIServerError(_('Unable to parse server response %d: %s') % (response.status_code, raw))
184:            raise APIServerError(content['error_message'], errorcode=content.get('error_code', None), inputcommand=path, detailInfoType=content.get('detailInfoType',None), detailInfo=content.get('detailInfo',None))
187:            raise APIServerError(content['error'].get('message', raw), inputcommand=path)
190:            raise APIServerError(_('Unexpected server response %d: %s') % (response.status_code, raw))
206:            raise APIServerError(_('Unexpected server response %d: %s') % (response.status_code, raw))
229:            raise ControllerGraphClientException(_('Unexpected server response %d: %s') % (statusCode, raw), statusCode=statusCode, response=response)
239:        # raise any error returned
245:            raise ControllerGraphClientException(message, statusCode=statusCode, content=content, response=response, errorCode=errorCode)
248:            raise ControllerGraphClientException(_('Unexpected server response %d: %s') % (statusCode, raw), statusCode=statusCode, response=response)

Perhaps this is better:

Exception class raised upon unsuccessful HTTP REST API requests.

I hope this might disambiguate the GraphQL errors.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hemangandhi I will take a look at it again

_response = None # http response that resulted in the error

def __init__(self, message='', response=None):
Expand All @@ -134,6 +136,7 @@ def __init__(self, message='', response=None):

@property
def response(self):
"""The http response that resulted in the error"""
return self._response


Expand Down