Skip to content

Conversation

@rihp
Copy link
Contributor

@rihp rihp commented Nov 21, 2022

The following PR implements the CCB feat with these functions need to be built:

  • build
  • get envs
  • Set_env
  • add_env
  • add_envs
  • add interface implementations
  • set interface implementations
  • add wrapper
  • add wrappers
  • remove wrapper
  • set_resolver
  • Readme

Update 21/11 :
Was able to implement the basic set of features needed to use the config builder,
Although all the tests seem to pass, we are receiving a typecheck error, which can be found here.
https://github.com/polywrap/python-client/actions/runs/3513623139/jobs/5886667818#step:6:31

Update 22/11 : This current set up should be an MVP of the package, ready for a first review

@rihp rihp changed the title ClientCongifBuilder ClientCongigBuilder Nov 21, 2022
@rihp rihp changed the title ClientCongigBuilder ClientConfigBuilder Nov 21, 2022
@rihp rihp self-assigned this Nov 21, 2022
@rihp
Copy link
Contributor Author

rihp commented Nov 22, 2022

The current build is passing, but there is a legacy module which seems to be failing (the polywrap-ccb is the one raising the error)

i´m looking into ways to fix this as other tests builds are passing correctly now

@rihp rihp marked this pull request as ready for review November 22, 2022 11:08
@rihp rihp requested review from Niraj-Kamdar and cbrzn November 22, 2022 11:08
@rihp rihp added the enhancement New feature or request label Nov 22, 2022
self.config.resolver = uri_resolver
return self

class ClientConfigBuilder(BaseClientConfigBuilder):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this was left in was that initially the ClientConfigBuilder is the object we call on the JS implementation

When we have defaults configs we can add them to this object for example

So the tests we instantiate
ccb = ClientConfigBuilder,

and not
bccb = BaseClientConfigBuilder.

I understand the BaseCCB is the benchmark functionality a client should have, while ClientConfigBuilder would be particular implementations of that BaseCCB

this also brings up the question on yesterday's call of how to structure the different client configurations.

Let me know if we should remove or rename them it and i'll update the classes and the tests to fit the correct implementation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, so what I would suggest would be the following:

  • In the BaseClientConfigBuilder let's implement the main methods (basically, all, except for build - this will be the only method without implementation)
  • In the ClientConfigBuilder we only implement build, inspired on this method from JS client. Note that currently the resolvers are not implemented so you can just return a mock object, let's just focus on the architecture now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for context, currently we have IClientConfigBuilder which is the abstract class, BaseClientConfigBuilder which is the one that implements and ClientConfigBuilder is just there to have the same API. With the suggested changes this will make that IClientConfigBuilder doesn't exists anymore and the BaseClientConfigBuilder will inherit from ABC (meaning that is the abstract class), here, we will implement all methods except build (making them abstract, in case the use that inherit from this class can define custom behavior); and ClientConfigBuilder will only implement build

@rihp rihp mentioned this pull request Nov 24, 2022
20 tasks
@rihp rihp closed this Nov 24, 2022
@rihp
Copy link
Contributor Author

rihp commented Nov 24, 2022

continued in #54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants