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

Conversation

Relativiteit
Copy link

@Relativiteit Relativiteit commented Apr 23, 2023

add type hinting, document strings and typo in readme Resolves #13
Please test on linux machine before merging!!

@Relativiteit
Copy link
Author

@rdiankov @ziyan @liuhuanjim013 @kanunikov-denys if no longer needed please close :)

@cielavenir
Copy link

Currently we need to maintain Python2 as well. This pull request can be considered at least 1 year later.

@Relativiteit
Copy link
Author

@cielavenir alright, is there anything else more pressing currently you need help on ?

@cielavenir
Copy link

@felixvd @hemangandhi could you check?

@ziyan
Copy link
Member

ziyan commented Oct 16, 2024

@cielavenir Do you have a pipeline ID?

Copy link

@hemangandhi hemangandhi left a comment

Choose a reason for hiding this comment

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

Thanks for the type annotations!

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?

@@ -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.

@cielavenir
Copy link

@ziyan https://tiny.mujin.co.jp/qarwm

However, since this typing will drop py2 explicitly, @felixvd must approve explicitly.

Copy link
Collaborator

@felixvd felixvd left a comment

Choose a reason for hiding this comment

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

We have other Python3-style type annotation now, so LGTM!

@cielavenir
Copy link

ok to merge after minor syntax is corrected (this is year-old mr so @hemangandhi do you want to take a look at this? i pushed the same name branch to mujin)

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

Successfully merging this pull request may close these issues.

5 participants