Skip to content
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

[Refactor] consistent naming for ClientConfigBuilder #31

Open
Niraj-Kamdar opened this issue May 11, 2023 · 4 comments
Open

[Refactor] consistent naming for ClientConfigBuilder #31

Niraj-Kamdar opened this issue May 11, 2023 · 4 comments

Comments

@Niraj-Kamdar
Copy link
Contributor

I think we should name, interfaceIClientConfigBuilder to ClientConfigBuilder and ClientConfigBuilder implementation to PolywrapClientConfigBuilder.

I believe, We should also rename addPackage, addWrapper to setPackage, setWrapper, etc. because add generally means adding something without overwriting while set will mean setting it and also conveys that it may overwrite. I think the usage of addInterfaceImplementations seems correct because we always add it to the list.

@pileks
Copy link
Contributor

pileks commented May 11, 2023

My thoughts:

I completely agree with the add and set naming. add is a remnant of before we used a Record/Map.

However, I don't agree with changing the name to PolywrapClientConfigBuilder, as its build method returns a CoreClientConfig object usable both by PolywrapClient and CoreClient.
Having a distinct I in IClientConfigBuilder always seemed like a good, idiomatic approach when we have its implementation living right next to it, and I feel like we should keep it that way, for the time being.

@Niraj-Kamdar
Copy link
Contributor Author

I respectfully disagree with your viewpoint. The debate regarding prefixing I in interface names has indeed been a topic of discussion, but the prevailing consensus tends to lean towards not using it. In fact, even within our codebase, we appear to be split between the two approaches, knowingly or unknowingly.

If we examine the examples of Wrapper, WrapPackage, and others in the CoreClient module, we can observe that they are all interfaces that do not have the I prefix. There is a valid reason for this choice. If we were to have IWrapper as the interface and Wrapper as its implementation, an obvious question would arise: what kind of wrapper does it refer to? By introducing the Wrapper interface alongside WasmWrapper and PluginWrapper as implementations, we can enhance clarity and readability in our codebase. The prefixing of I convention often stems from developers seeking an easy way out when faced with the task of finding a suitable name for both the interface and its implementation.

Considering the above, I still believe it would be beneficial to rename interface IClientConfigBuilder to ClientConfigBuilder and the ClientConfigBuilder implementation to either PolywrapClientConfigBuilder or something else. This change aligns with our ongoing efforts to use more descriptive and intuitive naming conventions.

I understand that you hold a different perspective, and I welcome further discussion on this matter to arrive at a consensus that best suits our codebase and development practices.

@pileks
Copy link
Contributor

pileks commented May 11, 2023

To be honest, I wasn't aware of that change, as I remember seeing IWrapPackage not so long ago! 😄

Given this information, I can agree with your proposal for the naming of the interface.

Additionally, let's first define which client does the current ClientConfigBuilder primarily serve? Is it the PolywrapCoreClient, or is it the PolywrapClient?

As it does seem to serve our PolywrapClient mainly, having it named PolywrapClientConfigBuilder makes sense.

I'd also love to hear more input, but this is all I really have to say about this matter. I'd go forward.

@Niraj-Kamdar
Copy link
Contributor Author

Don't worry! It's not uncommon for things to slip our minds in a dynamic and evolving codebase.

Based on your understanding and agreement with the proposed naming conventions, I'm glad to hear that you find the naming of the interface as PolywrapClientConfigBuilder appropriate. This aligns with the main client it serves and provides clear context within the codebase.

Thank you for sharing your thoughts on this matter, and I appreciate your support in moving forward with these changes. If any further input or concerns arise, I'm open to further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants