Refactor AWS Python Client #25
Conversation
|
Given that this is a client, and won't need serverless specifically to be tested, I can offer to provide an AWS instance of the API for any required testing. Ping me here or on discord and I'll push up a keyed version for you. |
| return client_id | ||
|
|
||
| def get_response(self, client_id): | ||
| with request.urlopen(f'{self.api_base}{client_id}?key={self.api_key}') as response: |
There was a problem hiding this comment.
Hi,
I know this is a refactoring PR and the issues exist in the original code, but should this and the request below be wrapped in a try/except URLError? or is the client just restarted if it crashes? Also below you are installing an opener on every call but it looks like that installs the opener globally so could that be moved outside the loop?
PS. I don't have write access but saw the PR while reading the code and thought I'd comment. I hope you don't mind.
There was a problem hiding this comment.
Heyo, yes they should be. Technically, since the client will be calling this from just a command line and it's not automated they can just restart it BUT I do agree that it's better to have the try/except there.
As for the opener, probably. I've barely used urllib and have always just stuck around with the requests library.
I'll push up a commit real quick to address these few things. If you notice anything else, feel free to comment. This was written in like 10 minutes, maybe less so there's bound to be errors.
(also, I don't mind at all :P)
| try: | ||
| request.urlretrieve(data.get('url') or '', 'image.tmp') | ||
| except URLError as e: | ||
| print('Failed to retrieve image: {e}') |
| data.get('title') or '', | ||
| data.get('message') or '', | ||
| WinApi.box_type(data.get('type') or ''), | ||
| data.get('icon') and WinApi.IconType[data['icon'].upper()] or None |
There was a problem hiding this comment.
This could raise a KeyError if the icon name isn't valid.
You could move it to a try/except or maybe
, getattr(WinApi.IconType, data.get('icon', '').upper(), None),
if you want to keep everything on one line.
| API_BASE = os.environ.get('AWS_CONTROL_API_BASE') or options.api_base | ||
|
|
||
| if not API_KEY or not API_BASE: | ||
| print('Invalid API_KEY/API_BASE set, please use `file.py -k <KEY> -b <BASE>` or set the `AWS_CONTROL_API_KEY/AWS_CONTROL_API_BASE` env vars!') |
There was a problem hiding this comment.
minor nit: you could use run.py or sys.argv[0] instead of file.py
A complete rewrite of the aws remote-control python client.
This allows easier expansion if there's ever any updates to the API & could even be abstracted more to allow for even better/easier expansion.
Currently, I was able to test it on Linux since that is my main OS but I wasn't really able to test it on Windows besides a quick test to see if the message boxes were working correctly.
I would appreciate someone who has Serverless & can test it out before it gets merged.