Repository navigation
[Refactor] consistent naming for ClientConfigBuilder #31
Description
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.
Activity
pileks commentedon May 11, 2023
My thoughts:
I completely agree with the
addandsetnaming.addis a remnant of before we used a Record/Map.However, I don't agree with changing the name to
PolywrapClientConfigBuilder, as itsbuildmethod returns aCoreClientConfigobject usable both byPolywrapClientandCoreClient.Having a distinct
IinIClientConfigBuilderalways 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 commentedon May 11, 2023
I respectfully disagree with your viewpoint. The debate regarding prefixing
Iin 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 theCoreClientmodule, we can observe that they are all interfaces that do not have theIprefix. There is a valid reason for this choice. If we were to haveIWrapperas the interface andWrapperas its implementation, an obvious question would arise: what kind of wrapper does it refer to? By introducing theWrapperinterface alongsideWasmWrapperandPluginWrapperas implementations, we can enhance clarity and readability in our codebase. The prefixing ofIconvention 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
IClientConfigBuildertoClientConfigBuilderand theClientConfigBuilderimplementation to eitherPolywrapClientConfigBuilderor 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 commentedon May 11, 2023
To be honest, I wasn't aware of that change, as I remember seeing
IWrapPackagenot 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 thePolywrapClient?As it does seem to serve our
PolywrapClientmainly, having it namedPolywrapClientConfigBuildermakes 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 commentedon May 11, 2023
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.