PR#2: Add logger and do all the http calls in one place - libpagure

libpagure

#2 Add logger and do all the http calls in one place

Merged 8 years ago by yangl1996. Opened 8 years ago by pingou.

pingou/libpagure all_calls_one_place into master

Rework the library to make all the http calls in one place

Pierre-Yves Chibon • 8 years ago

fe9c636

Add a logger so that we can log what's going on

Pierre-Yves Chibon • 8 years ago

d197584

libpagure/libpagure.py

file modified

+197 -89

		`@@ -1,10 +1,30 @@`
		`import requests`


		`+ class NullHandler(logging.Handler):`
		`+ ''' Null logger to avoid spurious messages`
		`+`
		`+ '''`
		`+ def emit(self, record):`
		`+ pass`
		`+`
		`+ LOG = logging.getLogger("libpagure")`
		`+`
		`+ # Add the null handler to top-level logger used by the library`
		`+ hand = NullHandler()`
		`+ LOG.addHandler(hand)`
		`+`
		`+`
		`class Pagure:`
		`# TODO: add error handling`
		`# TODO: write some unit tests`
		`- def __init__(self, pagure_token, pagure_repository, fork_username=None, instance_url="https://pagure.io"):`
		`+ def __init__(`
		`+ self,`
		`+ pagure_token,`
		`+ pagure_repository,`
		`+ fork_username=None,`
		`+ instance_url="https://pagure.io",`
		`+ insecure=False):`
		`"""`
		`Create an instance.`
		`:param pagure_token: pagure API token`
		`@@ -18,6 +38,49 @@`
		`self.ForkUsername = fork_username`
		`self.InstanceURL = instance_url`
		`self.Header = {"Authorization": "token " + self.Token}`
		`+ self.session = requests.session()`
		`+ self.insecure = insecure`
		`+`
		`+ def __call_api(url, method='GET', params=None, data=None):`
		`+ """ Private method used to call the API.`
		`+ It returns the raw JSON returned by the API or raises an exception`
		`+ if something goes wrong.`
		`+`
		`+ :arg url: the URL to call`
		`+ :kwarg method: the HTTP method to use when calling the specified`
		`+ URL, can be GET, POST, DELETE, UPDATE...`
		`+ Defaults to GET`
		`+ :kwarg params: the params to specify to a GET request`
		`+ :kwarg data: the data to send to a POST request`
		`+`
		`+ """`
		`+`
		`+ req = self.session.request(`
		`+ method=method,`
		`+ url=url,`
		`+ params=params,`
		`+ headers=self.headers,`
		`+ data=data,`
		`+ verify=not self.insecure,`
		`+ )`
		`+ self._save_cookies()`
		`+`
		`+ output = None`
		`+ try:`
		`+ output = req.json()`
		`+ except Exception, err:`
		`+ LOG.debug(req.text)`
		`+ # TODO: use a dedicated error class`
		`+ raise Exception('Error while decoding JSON: {0}'.format(err))`
		`+`
		`+ if req.status_code != 200:`
		`+ LOG.debug('full output: {0}'.format(output))`
		`+ if output is None:`
		`+ # TODO: use a dedicated error class`
		`+ raise Exception(`
		`+ 'No output returned by %s' % response.url)`
		`+`
		`+ return output`

		`def api_version(self):`
		`"""`
		`@@ -25,8 +88,7 @@`
		`:return:`
		`"""`
		`request_url = "{}/api/0/version".format(self.InstanceURL)`
		`- r = requests.get(request_url, headers=self.Header)`
		`- return_value = r.json()`
		`+ return_value = __call_api(request_url)`
		`return return_value['version']`

		`def list_users(self, pattern=None):`
		`@@ -36,11 +98,11 @@`
		`:return:`
		`"""`
		`request_url = "{}/api/0/users".format(self.InstanceURL)`
		`- if pattern is None:`
		`- r = requests.get(request_url, headers=self.Header)`
		`- else:`
		`- r = requests.get(request_url, params={'pattern': pattern})`
		`- return_value = r.json()`
		`+ return_value = __call_api(request_url)`
		`+ params = None`
		`+ if pattern:`
		`+ params = {'pattern': pattern}`
		`+ return_value = __call_api(request_url, params=params)`
		`return return_value['users']`

		`def list_tags(self, pattern=None):`
		`@@ -50,14 +112,16 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/tags".format(self.InstanceURL, self.Repository)`
		`+ request_url = "{}/api/0/{}/tags".format(`
		`+ self.InstanceURL, self.Repository)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/tags".format(self.InstanceURL, self.ForkUsername, self.Repository)`
		`- if pattern is None:`
		`- r = requests.get(request_url, headers=self.Header)`
		`- else:`
		`- r = requests.get(request_url, headers=self.Header, params={'pattern': pattern})`
		`- return_value = r.json()`
		`+ request_url = "{}/api/0/fork/{}/{}/tags".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository)`
		`+ params = None`
		`+ if pattern:`
		`+ params = {'pattern': pattern}`
		`+`
		`+ return_value = __call_api(request_url, params=params)`
		`return return_value['tags']`

		`def list_groups(self, pattern=None):`
		`@@ -67,11 +131,11 @@`
		`:return:`
		`"""`
		`request_url = "{}/api/0/groups".format(self.InstanceURL)`
		`- if pattern is None:`
		`- r = requests.get(request_url, headers=self.Header)`
		`- else:`
		`- r = requests.get(request_url, headers=self.Header, params={'pattern': pattern})`
		`- return_value = r.json()`
		`+ params = None`
		`+ if pattern:`
		`+ params = {'pattern': pattern}`
		`+`
		`+ return_value = __call_api(request_url, params=params)`
		`return return_value['groups']`

		`def error_codes(self):`
		`@@ -79,9 +143,8 @@`
		`Get a dictionary of all error codes.`
		`:return:`
		`"""`
		`- request_url = "{}/api/0/error_codes"`
		`- r = requests.get(request_url, headers=self.Header)`
		`- return_value = r.json()`
		`+ request_url = "{}/api/0/error_codes".format(self.InstanceURL)`
		`+ return_value = __call_api(request_url)`
		`return return_value`

		`def list_requests(self, status=None, assignee=None, author=None):`
		`@@ -93,10 +156,11 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/pull-requests".format(self.InstanceURL, self.Repository)`
		`+ request_url = "{}/api/0/{}/pull-requests".format(`
		`+ self.InstanceURL, self.Repository)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/pull-requests".format(self.InstanceURL, self.ForkUsername,`
		`- self.Repository)`
		`+ request_url = "{}/api/0/fork/{}/{}/pull-requests".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository)`
		`payload = {}`
		`if status is not None:`
		`payload['status'] = status`
		`@@ -104,8 +168,8 @@`
		`payload['assignee'] = assignee`
		`if author is not None:`
		`payload['author'] = author`
		`- r = requests.get(request_url, headers=self.Header, params=payload)`
		`- return_value = r.json()`
		`+`
		`+ return_value = __call_api(request_url, params=payload)`
		`return return_value['requests']`

		`def request_info(self, request_id):`
		`@@ -115,12 +179,14 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/pull-request/{}".format(self.InstanceURL, self.Repository, request_id)`
		`+ request_url = "{}/api/0/{}/pull-request/{}".format(`
		`+ self.InstanceURL, self.Repository, request_id)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/pull-request/{}".format(self.InstanceURL, self.ForkUsername,`
		`- self.Repository, request_id)`
		`- r = requests.get(request_url, headers=self.Header)`
		`- return_value = r.json()`
		`+ request_url = "{}/api/0/fork/{}/{}/pull-request/{}".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository,`
		`+ request_id)`
		`+`
		`+ return_value = __call_api(request_url)`
		`return return_value`

		`def merge_request(self, request_id):`
		`@@ -130,12 +196,15 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/pull-request/{}/merge".format(self.InstanceURL, self.Repository, request_id)`
		`+ request_url = "{}/api/0/{}/pull-request/{}/merge".format(`
		`+ self.InstanceURL, self.Repository, request_id)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/pull-request/{}/merge".format(self.InstanceURL, self.ForkUsername,`
		`- self.Repository, request_id)`
		`- r = requests.post(request_url, headers=self.Header)`
		`- return_value = r.json()`
		`+ request_url = "{}/api/0/fork/{}/{}/pull-request/{}/merge".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository,`
		`+ request_id)`
		`+`
		`+ return_value = __call_api(request_url, method='POST')`
		`+`
		`if return_value['message'] == "Changes merged!":`
		`result = (True, return_value['message'])`
		`else:`
		`@@ -149,12 +218,15 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/pull-request/{}/close".format(self.InstanceURL, self.Repository, request_id)`
		`+ request_url = "{}/api/0/{}/pull-request/{}/close".format(`
		`+ self.InstanceURL, self.Repository, request_id)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/pull-request/{}/close".format(self.InstanceURL, self.ForkUsername,`
		`- self.Repository, request_id)`
		`- r = requests.post(request_url, headers=self.Header)`
		`- return_value = r.json()`
		`+ request_url = "{}/api/0/fork/{}/{}/pull-request/{}/close".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository,`
		`+ request_id)`
		`+`
		`+ return_value = __call_api(request_url, method='POST')`
		`+`
		`if return_value['message'] == "Pull-request closed!":`
		`result = (True, return_value['message'])`
		`else:`
		`@@ -172,10 +244,13 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/pull-request/{}/comment".format(self.InstanceURL, self.Repository, request_id)`
		`+ request_url = "{}/api/0/{}/pull-request/{}/comment".format(`
		`+ self.InstanceURL, self.Repository, request_id)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/pull-request/{}/comment".format(self.InstanceURL, self.ForkUsername,`
		`- self.Repository, request_id)`
		`+ request_url = "{}/api/0/fork/{}/{}/pull-request/{}/comment".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository,`
		`+ request_id)`
		`+`
		`payload = {'comment': body}`
		`if commit is not None:`
		`payload['commit'] = commit`
		`@@ -183,8 +258,9 @@`
		`payload['filename'] = filename`
		`if row is not None:`
		`payload['row'] = row`
		`- r = requests.post(request_url, data=payload, headers=self.Header)`
		`- return_value = r.json()`
		`+`
		`+ return_value = __call_api(request_url, method='POST', data=payload)`
		`+`
		`if return_value['message'] == "Comment added":`
		`result = (True, return_value['message'])`
		`else:`
		`@@ -204,17 +280,21 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/pull-request/{}/flag".format(self.InstanceURL, self.Repository, request_id)`
		`+ request_url = "{}/api/0/{}/pull-request/{}/flag".format(`
		`+ self.InstanceURL, self.Repository, request_id)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/pull-request/{}/flag".format(self.InstanceURL, self.ForkUsername,`
		`- self.Repository, request_id)`
		`+ request_url = "{}/api/0/fork/{}/{}/pull-request/{}/flag".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository,`
		`+ request_id)`
		`+`
		`payload = {'username': username, 'percent': percent, 'comment': comment, 'url': url}`
		`if commit is not None:`
		`payload['commit'] = commit`
		`if uid is not None:`
		`payload['uid'] = uid`
		`- r = requests.post(request_url, data=payload, headers=self.Header)`
		`- return_value = r.json()`
		`+`
		`+ return_value = __call_api(request_url, method='POST', data=payload)`
		`+`
		`if return_value['message'] == "Flag added" or return_value['message'] == "Flag updated":`
		`result = (True, return_value['message'])`
pingou commented 8 years ago Btw, I think we should avoid these, returning a tuple in a library is very tricky it's not really something people expect to retrieve. If we keep it, it should be really well documented (Note: I place the comment here, but this is valid elsewhere)
yangl1996 commented 8 years ago Yes, users won't expect this type of return value. Do you have some ideas about how shall we change it? I think it would be helpful if users can know why was the operation failed :)
		`else:`
		`@@ -230,14 +310,18 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/new_issue".format(self.InstanceURL, self.Repository)`
		`+ request_url = "{}/api/0/{}/new_issue".format(`
		`+ self.InstanceURL, self.Repository)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/new_issue".format(self.InstanceURL, self.ForkUsername, self.Repository)`
		`+ request_url = "{}/api/0/fork/{}/{}/new_issue".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository)`
		`+`
		`payload = {'title': title, 'issue_content': content}`
		`if private is not None:`
		`payload['private'] = private`
		`- r = requests.post(request_url, data=payload, headers=self.Header)`
		`- return_value = r.json()`
		`+`
		`+ return_value = __call_api(request_url, method='POST', data=payload)`
		`+`
		`if return_value['message'] == "Issue created":`
		`result = (True, return_value['message'])`
		`else:`
		`@@ -254,9 +338,12 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/issues".format(self.InstanceURL, self.Repository)`
		`+ request_url = "{}/api/0/{}/issues".format(`
		`+ self.InstanceURL, self.Repository)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/issues".format(self.InstanceURL, self.ForkUsername, self.Repository)`
		`+ request_url = "{}/api/0/fork/{}/{}/issues".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository)`
		`+`
		`payload = {}`
		`if status is not None:`
		`payload['status'] = status`
		`@@ -266,8 +353,9 @@`
		`payload['assignee'] = assignee`
		`if author is not None:`
		`payload['author'] = author`
		`- r = requests.get(request_url, params=payload, headers=self.Header)`
		`- return_value = r.json()`
		`+`
		`+ return_value = __call_api(request_url, params=payload)`
		`+`
		`return return_value['issues']`

		`def issue_info(self, issue_id):`
		`@@ -277,11 +365,15 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/issue/{}".format(self.InstanceURL, self.Repository, issue_id)`
		`+ request_url = "{}/api/0/{}/issue/{}".format(`
		`+ self.InstanceURL, self.Repository, issue_id)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/issue/{}".format(self.InstanceURL, self.ForkUsername, self.Repository, issue_id)`
		`- r = requests.get(request_url, headers=self.Header)`
		`- return_value = r.json()`
		`+ request_url = "{}/api/0/fork/{}/{}/issue/{}".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository,`
		`+ issue_id)`
		`+`
		`+ return_value = __call_api(request_url, params=payload)`
		`+`
		`return return_value`

		`def get_list_comment(self, issue_id, comment_id):`
		`@@ -292,13 +384,15 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/issue/{}/comment/{}".format(self.InstanceURL, self.Repository, issue_id,`
		`- comment_id)`
		`+ request_url = "{}/api/0/{}/issue/{}/comment/{}".format(`
		`+ self.InstanceURL, self.Repository, issue_id, comment_id)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/issue/{}/comment/{}".format(self.InstanceURL, self.ForkUsername,`
		`- self.Repository, issue_id, comment_id)`
		`- r = requests.get(request_url, headers=self.Header)`
		`- return_value = r.json()`
		`+ request_url = "{}/api/0/fork/{}/{}/issue/{}/comment/{}".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository,`
		`+ issue_id, comment_id)`
		`+`
		`+ return_value = __call_api(request_url, params=payload)`
		`+`
		`return return_value`

		`def change_issue_status(self, issue_id, new_status):`
		`@@ -309,13 +403,17 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/issue/{}/status".format(self.InstanceURL, self.Repository, issue_id)`
		`+ request_url = "{}/api/0/{}/issue/{}/status".format(`
		`+ self.InstanceURL, self.Repository, issue_id)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/issue/{}/status".format(self.InstanceURL, self.ForkUsername,`
		`- self.Repository, issue_id)`
		`+ request_url = "{}/api/0/fork/{}/{}/issue/{}/status".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository,`
		`+ issue_id)`
		`+`
		`payload = {'status': new_status}`
		`- r = requests.post(request_url, data=payload, headers=self.Header)`
		`- return_value = r.json()`
		`+`
		`+ return_value = __call_api(request_url, method='POST', data=payload)`
		`+`
		`if return_value['message'].startswith("Successfully"):`
		`result = (True, return_value['message'])`
		`else:`
		`@@ -330,13 +428,17 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/issue/{}/comment".format(self.InstanceURL, self.Repository, issue_id)`
		`+ request_url = "{}/api/0/{}/issue/{}/comment".format(`
		`+ self.InstanceURL, self.Repository, issue_id)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/issue/{}/comment".format(self.InstanceURL, self.ForkUsername,`
		`- self.Repository, issue_id)`
		`+ request_url = "{}/api/0/fork/{}/{}/issue/{}/comment".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository,`
		`+ issue_id)`
		`+`
		`payload = {'comment': body}`
		`- r = requests.post(request_url, data=payload, headers=self.Header)`
		`- return_value = r.json()`
		`+`
		`+ return_value = __call_api(request_url, method='POST', data=payload)`
		`+`
		`if return_value['message'] == 'Comment added':`
		`result = (True, return_value['message'])`
		`else:`
		`@@ -349,11 +451,14 @@`
		`:return:`
		`"""`
		`if self.ForkUsername is None:`
		`- request_url = "{}/api/0/{}/git/tags".format(self.InstanceURL, self.Repository)`
		`+ request_url = "{}/api/0/{}/git/tags".format(`
		`+ self.InstanceURL, self.Repository)`
		`else:`
		`- request_url = "{}/api/0/fork/{}/{}/git/tags".format(self.InstanceURL, self.ForkUsername, self.Repository)`
		`- r = requests.get(request_url, headers=self.Header)`
		`- return_value = r.json()`
		`+ request_url = "{}/api/0/fork/{}/{}/git/tags".format(`
		`+ self.InstanceURL, self.ForkUsername, self.Repository)`
		`+`
		`+ return_value = __call_api(request_url)`
		`+`
		`return return_value['tags']`

		`def list_projects(self, tags=None, username=None, fork=None):`
		`@@ -365,6 +470,7 @@`
		`:return:`
		`"""`
		`request_url = "{}/api/0/projects".format(self.InstanceURL)`
		`+`
		`payload = {}`
		`if tags is not None:`
		`payload['tags'] = tags`
		`@@ -372,8 +478,9 @@`
		`payload['username'] = username`
		`if fork is not None:`
		`payload['fork'] = fork`
		`- r = requests.get(request_url, params=payload, headers=self.Header)`
		`- return_value = r.json()`
		`+`
		`+ return_value = __call_api(request_url, params=payload)`
		`+`
		`return return_value['projects']`

		`def user_info(self, username):`
		`@@ -383,6 +490,7 @@`
		`:return:`
		`"""`
		`request_url = "{}/api/0/user/{}".format(self.InstanceURL, username)`
		`- r = requests.get(request_url, headers=self.Header)`
		`- return_value = r.json()`
		`+`
		`+ return_value = __call_api(request_url)`
		`+`
		`return return_value`

no initial comment

pingou commented on line 305 of libpagure/libpagure.py 8 years ago

Btw, I think we should avoid these, returning a tuple in a library is very tricky it's not really something people expect to retrieve.

If we keep it, it should be really well documented

(Note: I place the comment here, but this is valid elsewhere)

yangl1996 commented on line 305 of libpagure/libpagure.py 8 years ago

Yes, users won't expect this type of return value. Do you have some ideas about how shall we change it? I think it would be helpful if users can know why was the operation failed :)

yangl1996 commented 8 years ago

Learned a lot from it, thank you very much!

I will merge this now and modify the return value of the POST methods, changing the return value from tuple to a dictionary, and modify the function description strings ;)

pingou commented 8 years ago

I will merge this now and modify the return value of the POST methods, changing the return value from tuple to a dictionary, and modify the function description strings ;)

I don't think this is the right approach, I can think of two ways:

1/ just return the message and let the user deal with it. After all most of the
time the consumer will just display the message and let the user (human) deal
with it.

2/ check the returned message and raise an exception when something went wrong.
The message of the exception could be the content of the returned message, thus
the description of the error.

yangl1996 commented 8 years ago

I will merge this now and modify the return value of the POST methods, changing the return value from tuple to a dictionary, and modify the function description strings ;)

I don't think this is the right approach, I can think of two ways:
1/ just return the message and let the user deal with it. After all most of the
time the consumer will just display the message and let the user (human) deal
with it.
2/ check the returned message and raise an exception when something went wrong.
The message of the exception could be the content of the returned message, thus
the description of the error.

I think you are right. We should return straightforward results. I've implemented method 2, when there is no error, it returns "True" and otherwise raises an error containing the return message.

pingou commented 8 years ago

I will merge this now and modify the return value of the POST methods, changing the return value from tuple to a dictionary, and modify the function description strings ;)

I don't think this is the right approach, I can think of two ways:
1/ just return the message and let the user deal with it. After all most of the
time the consumer will just display the message and let the user (human) deal
with it.
2/ check the returned message and raise an exception when something went wrong.
The message of the exception could be the content of the returned message, thus
the description of the error.

I think you are right. We should return straightforward results. I've implemented method 2, when there is no error, it returns "True" and otherwise raises an error containing the return message.

The question becomes: is there an interest of the method to return something?

It could simply be, everything works and it's cool or we raise an exception.
Return True in this case doesn't bring any information over returning None (ie
not returning anything)

yangl1996 commented 8 years ago

I will merge this now and modify the return value of the POST methods, changing the return value from tuple to a dictionary, and modify the function description strings ;)
I don't think this is the right approach, I can think of two ways:
1/ just return the message and let the user deal with it. After all most of the
time the consumer will just display the message and let the user (human) deal
with it.
2/ check the returned message and raise an exception when something went wrong.
The message of the exception could be the content of the returned message, thus
the description of the error.

I think you are right. We should return straightforward results. I've implemented method 2, when there is no error, it returns "True" and otherwise raises an error containing the return message.

The question becomes: is there an interest of the method to return something?
It could simply be, everything works and it's cool or we raise an exception.
Return True in this case doesn't bring any information over returning None (ie
not returning anything)

Yes, I agree with you :) There is no obvious need for an actual return value. I've updated it.