-
Notifications
You must be signed in to change notification settings - Fork 3
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
renamed PolywrapClient to Client #234
Conversation
Codecov ReportPatch coverage is
📢 Thoughts on this report? Let us know!. |
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 is awesome! thanks for pushing this 🙏
do you think we should also change the PolywrapClientConfigBuilder
to ClientConfigBuilder
? As well as PolywrapClientConfig
to ClientConfig
? (In another PR ofc)
Yes, that make sense. I'll make those changes! |
…Builder to ClientConfigBuilder
|
||
#[derive(Default, Clone)] | ||
pub struct PolywrapClientConfig { | ||
pub struct ClientConfig { |
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.
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 think we should make the most user-facing type the easiest to understand and read. So, that would mean that the config used by the builder is the ClientConfig
, and the "fully built config" that's used by the client would be the CoreClientConfig
.
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 think we should make the most user-facing type the easiest to understand and read. So, that would mean that the config used by the builder is the
ClientConfig
, and the "fully built config" that's used by the client would be theCoreClientConfig
.
yah in my opinion that's not the easiest to understand and read but rather confusing. if I see this for the first time I would ask to myself "what's the difference between a client and a core client" and why do they have different attributes. from my perspective, this can be really simplified to say that one is the builder attributes and the other is the client attributes (but again, this is my opinion and I can be completely wrong)
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.
anyways, I will merge this because it is a nit, and there's no need to block this
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.
LGTM! 🔥
Closes #236