-
Notifications
You must be signed in to change notification settings - Fork 0
The great refactor [EWB-4557] #43
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
base: main
Are you sure you want to change the base?
Conversation
|
|
vincewhite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not full review. Just random comments.
| ## [0.23.0] - UNRELEASED | ||
| ### Breaking Changes | ||
| * None. | ||
| * EasClient will now need `auth=` passed with an auth object. either `BaseAuthMethod` or `TokenAuth`, this allows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This task should probably include the PR to update to this happy little repo: https://github.com/zepben/hosting-capacity-runner , maybe https://github.com/zepben/ewb-sdk-examples-python as well?
|
|
||
| def get_hosting_capacity_calibration_run(self, id: str): | ||
| def get_hosting_capacity_calibration_run(self, id_: str) -> dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably has to be mentioned as a breaking change?
| :param client_id: The Auth0 client ID used to specify to the auth server which application to request a token for. | ||
| :param client_secret: The Auth0 client secret used for M2M authentication. | ||
| """ | ||
| ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this magic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allows you to define multiple combinations of var/types to pass to the non @overloaded init.
Makes type hinting pick up when people have put a bad combination of auth params that would throw an error anyway.
f720255 to
b792cba
Compare
b3c5406 to
7fd17c8
Compare
7fd17c8 to
cc34b79
Compare
Signed-off-by: Max Chesterfield <[email protected]> tests pass Signed-off-by: Max Chesterfield <[email protected]>
Signed-off-by: Max Chesterfield <[email protected]>
Signed-off-by: Max Chesterfield <[email protected]>
cc34b79 to
e0222db
Compare
vincewhite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
half review, run out of time.
| from zepben.eas.client.auth_method import TokenAuth | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| from zepben.eas.client.auth_method import TokenAuth |
| json_serialiser=None | ||
|
|
||
|
|
||
| class BaseAuthMethod: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this is actually the connection that an authenticated connection is an extension on top of? To connect to an EAS without authentication I'm still required to pass a auth = BaseAuthMethod(host = ...) since it holds the connection/EAS host details.
This also makes it less optional than is suggested in the eas_client.py
| elif not response.ok: | ||
| response.raise_for_status() | ||
|
|
||
| def _do_request(self): # TODO: can this just be called once per run? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely
| response.raise_for_status() | ||
|
|
||
| @catch_warnings | ||
| async def _do_get_request(self, *, run_id: int): # TODO: Terrible name probably |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least there is a matching _do_post_request(), for _get_request_headers() you just have to hope _get doesn't refer to HTTP GET.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but also this seems hardcoded to retrieve opendss-models so yeh...
| return get_event_loop().run_until_complete(self.async_get_opendss_model(model_id)) | ||
|
|
||
| async def async_get_opendss_model(self, model_id: int): | ||
| async def async_get_opendss_model(self, model_id: int) -> dict: # TODO: this logic should be hidden behind a generator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
someone should update the backend to provide this functionality...
|
|
||
| @dataclass | ||
| class HostingCapacityDataclass(ABC): # TODO: Another terrible name | ||
| _snake_to_camel_overrides = dict( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyway these can be tested to confirm someone didn't forget to add an override when they were adding new fields.
| def __del__(self): | ||
| self.close() | ||
|
|
||
| def __enter__(self): | ||
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||
| self.close() | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume these call close when this the object is deleted/unneeded etc/used by other things. Should we be closing the session if it was passed into the eas client/would this be the least unexpected behaviour?
Description
Attempt at de-duping, tidying, and a slight restructure. Alot of the query body is now self generated by the objects themselves, I think its easily extendable, but im probably biased.
Associated tasks
None.
Test Steps
any code that worked before, should work exactly the same aside from needing the eas_client constructor to be changed to include auth
Checklist
If any of these are not applicable, strikethrough the line
~like this~. Do not delete it!. Let the reviewer decide if you should have done it.Code
Security
When developing applications, use following guidelines for information security considerations:
Documentation
Breaking Changes
Please leave a summary of the breaking changes here and then post it on the Slack breaking-changes channel to notify the team about it.
auth=passed with an auth object. eitherBaseAuthMethodorTokenAuth, this allows cleaner documenting of accepted constructor arguments.