BES API Client #1
Conversation
tallus
commented
May 8, 2017
tallus
commented
- client for accessing the BES API v1 & v2
- tests
There was a problem hiding this comment.
The only py3 issues I can across were the basestring problem we talked about and two uses of iteritems, both noted in line comments. If those changes are made then all tests, other than the one noted problem test, pass in py 2.7, 3.4, 3.5 and 3.6. I have a tox setup and ready to add if you want.
Pylint flags several whitespace errors, but it looks like most of the remaining pylint issues are by choice and may benefit from local overrides.
| # Imports from External Modules | ||
| import requests | ||
|
|
||
| # Config/Constants |
There was a problem hiding this comment.
I like your sys.version_info approach to the basestring, py3, compatibility issue, but what about simplifying it further to:
PY3 = sys.version_info[0] == 3
if PY3:
basestring = str
pybes/tests/test_pybes.py
Outdated
| self.client._check_call_success(mock_response, prefix='Test') | ||
| error = conm.exception | ||
| self.assertEqual( | ||
| error.message, 'Test: 404 test: error, error: test' |
There was a problem hiding this comment.
this test is problematic. It passes sometimes and fails others. I believe it is because sometimes the error message is
'Test: 404 test: error, error: test' and other times it is 'Test: 404 error: test, test: error' since the portion of the message after '404' is constructed from an unordered dict
pybes/pybes.py
Outdated
| # Private Functions and Classes | ||
| def _fix_params(params): | ||
| """For v1 api -- True is True but False is a string""" | ||
| for key, val in params.iteritems(): |
There was a problem hiding this comment.
so far, the iteritems calls seem to be the only other py3 incompatibility I have found. We can run the 2to3 migration tool if you like, but since you only have this in two place, plus one mention in a docstring, they could just manually be changed to
iter(parms.items())
There was a problem hiding this comment.
using items is probably sufficient here
pybes/pybes.py
Outdated
| ) | ||
| raise BESError(msg) | ||
| return { | ||
| key: val for key, val in dct.iteritems() |
There was a problem hiding this comment.
the other iteritems() usage. see above.
iter(dct.items())
pybes/pybes.py
Outdated
| Set up Client: | ||
|
|
||
| Note for everything except setting up a user you will need to supply | ||
| email, password and organization_token in order to authenticate. |
There was a problem hiding this comment.
While I can see you are implementing this correctly, this docstring reads as counter to BES documentation which states that the the organization_token is only used for creating a user and getting an access token for that user, from which point it is the access token that you need
There was a problem hiding this comment.
I added the ability to reuse tokens later as api calls are rate limiteds, I will update the docs accordingly.
pybes/pybes.py
Outdated
| :type float: | ||
| :param dimension_2: required Length of dimension 2 (in feet). | ||
| :type float: | ||
| :param /dimension_3: optional Length of dimension 3 (in feet). |
There was a problem hiding this comment.
there is a '/' character in front of the 'dimension_3' param name. also several param :type : blocks in this docstring need to have the the param name added, as in
:type vertices: str
rather than
:type str:
pybes/pybes.py
Outdated
| :type bool: | ||
|
|
||
| :rtype: None | ||
| :raises: BES/APIError |
There was a problem hiding this comment.
repeat of docstring notes from previous method
pybes/pybes.py
Outdated
| For convienience block_resources e.g. air_handler, 'air handler' | ||
| will be converted (ie. spaces will be converted to underscores | ||
| and the value will be converted to lower case and pluralized | ||
| if needed). block_ at the beginning may be ommited |
There was a problem hiding this comment.
typo in convenience and omitted
in param list, typo in resource_id in name block and in kwargs in type block.
should :name and :rname be :type and :rtype?
pybes/pybes.py
Outdated
| For convienience block_resource e.g. air_handler, 'air handler' | ||
| will be converted (ie. spaces will be converted to underscores | ||
| and the value will be converted to lower case and pluralized | ||
| if needed). block_ at the beginning may be ommited |
There was a problem hiding this comment.
typo in convenience and omitted again and the :name vs :type
same docstring issues for get_block_resource, get_block_resources, and update_block_resource below
| Update a building (using v1 api) | ||
| :param id: building id | ||
| :type id: int | ||
| :param name: name of building |
There was a problem hiding this comment.
no 'name' param in signature
|
Fixes in. |
