Builder pattern methods

[matkoch]

Throughout the code base there are many methods looking like builder pattern methods. I have three concerns with that:

1. Not all properties have corresponding builder methods. I'd suggest to make them mandatory, thus consistent, but it's not the most important thing :)
2. They are inconsistent in naming. MSBuildSetting has 3 different namings already (Set, With, and Use). I suggest to agree on a common naming, and change the existing methods (with a new major version; and maybe issue warnings upfront?!).
3. They are not returning a new object. Let me explain why returning a new instance is important/useful:
   • First, the builder pattern should work like that. So the status quo is a bit surprising.
   • Secondly, it supports use cases where a common instance is shared with multiple invocations of the same task

var commonNuGetPackSettings = new NuGetPackSettings {
    Authors = new[] { "..." },
    Owners = new[] { "..." },
    Symbols = true,
    BasePath = "...",
    OutputDirectory = "...",
    ... // many more
}
var firstPackageSettings = commonNuGetPackSettings
    .WithFiles(...);
var secondPackageSettings = commonNuGetPackSettings
    .WithFiles(...);

I hope this outlines the benefit good enough. It also applies to MSBuildSettings, for instance if you want to separate Compile from CodeAnalysis for reasons of parallelism. Please note that the builder pattern doesn't necessarily require immutability.

[matkoch]

This received quite some approval. I think it is a general design decision that should be made, also for new code. @patriksvensson, @phillipsj, I remember you guys had something to add, so I'd like to kindly bump this issue.

[phillipsj]

I can't officially speak for the team, but my recommendation, and the one by @patriksvensson, is to create an addin with the additional functionality. This will give everyone access to it and will also serve as a proof of concept. It would also help clarify it to others how it works

[matkoch]

@phillipsj actually, this is no additional functionality as you said. The methods are already there. Both, with inconsistent naming and surprising behavior. I don't think it's a good idea to put this into an add-in. The builder pattern is a well-known and straightforward pattern. Nothing fancy about it. Just a fluent way of modifying properties. However, splitting only guarantees a harder time finding that functionality.

[patriksvensson]

@matkoch Yes, there are some inconsistencies, but that is something we have to live with. We should however not introduce more inconsistencies in the code base without thinking it through how we want to APIs to be used. That's why it's better to battle test API improvements as addins until we know that is the way we want to go. (ping @phillipsj)

[phillipsj]

@matkoch I am participating in something very similar with the MSBuild 15 (Visual Studio 2017) integration. It is a very conservative approach, but the Cake API has been very stable, IME, and that is why they are taking this approach. I know for a fact that I would more than likely use your addin in some of my builds.

[matkoch]

Sorry but I don't understand 🤔 So far your response has been do it as an addin. But this doesn't solve anything.

First, it doesn't tackle the status quo. You've mentioned a stable API. At the same time you should ask yourself if you want to carry this burden with you. There will always be people asking:

Okay, this is called UseXXX, and that WithXXX... What's the difference?

or more annoyingly

WithToolsVersion ... ah fuck ... UseToolVersion.

Breaking an API is per se nothing bad. You can always help fixing the problem early upfront by issuing warnings (ObsoleteAttribute). Plus - UseToolVersion as an example - there are usage counts of around 150 on GitHub. I think under those circumstances, changes are very acceptable.

Second and more importantly, it doesn't encourage any guidelines for the future. Given the fact that addins can be created any time by any developer, we will always end up with inconsistencies. Again and again. And because there is no consistency, you'll either never integrate any of them, or introduce breaking changes for those, who have been using the addins.

not introduce more ... without thinking it through

Delegating to 3rd parties, doesn't mean to think it through. After all, I've tried very hard to make this issue the place of thinking it through 😄

[patriksvensson]

@matkoch I think we use With when you can set multiple values and Use when it's a single value.

[matkoch]

@patriksvensson can you give an example? You mean multiple arguments?

What about the return a new instance stuff? Does it make sense for you?

[devlead]

@matkoch in general reasoning behind starting as an addin/module is that we can test & bake the API before we merge it into the main project. Addins can also be reved more quickly and in a safe way without risking bloating or creating unnecessary breaking changes in the core,
And if it proves to be consensus around the API then we can pull in a fully baked API. This is especially good when weren't 100% sure yet on value of refactoring.

[patriksvensson]

@matkoch Example:

var settings = new MSBuildSettings()
   .UseToolVersion(..)
   .WithTarget("Clean")
   .WithTarget("Build");

Not sure I agree with the return a new instance stuff. Will need to think about it some more and talk with the rest of the @cake-build/team about it since it would require this change for all tools, and it's too big of a change. 

Changes to this approach could be implemented as addins and make it opt-in for now.