-
Notifications
You must be signed in to change notification settings - Fork 6
Refactor Env and Interfaces from ClientConfig base class #44
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
Conversation
|
The build is now passing, all green after the Env interface refactor 😎 now will look at the interfaces refactor |
…ihp/client_config_interfaces
| if isinstance(key, str): | ||
| dictionary[key] = sanitize(val) | ||
| else: | ||
| elif key.uri: |
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'm not familiar with this codebase, but why do we want to check whether the key.uri exists here? This msgpack package should be independent of any of the polywrap specific semantics, so referencing "uri" doesn't quite make sense here to me.
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.
It shouldn't be done like this, I will update it.
| def get_env_from_uri_history( | ||
| uri_history: List[Uri], client: Client | ||
| ) -> Union[Env, None]: | ||
| ) -> Union[Dict[str, Any], None]: |
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.
Why not use the Env type here? It helps me better understand what this code is doing.
Niraj-Kamdar
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.
I will make the requested changes and some refactor
packages/polywrap-core/polywrap_core/types/interface_implementation.py
Outdated
Show resolved
Hide resolved
| if isinstance(key, str): | ||
| dictionary[key] = sanitize(val) | ||
| else: | ||
| elif key.uri: |
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.
It shouldn't be done like this, I will update it.

Initially we had a ClientConfig base class with a list of Environments, and a list of interfaces,
Both of these fields are now being changed to a Dictionary for simpler client configuration.
This changes break some of the client functionality, working on those fixes now
Status update 10/nov/2022:
Was able to get all tests from Client package passing but broke some other packages like msgpack and core at build time.
I'm fixing those up next
Status update 11/nov/2022: