Fix lazy string format

[rokonec]

In couple of places we have not leveraged LazyFormattedBuildEventArgs properly.
With these changes strings would not be necessary to format, in cases when logging is limited like /clp:PerformanceSummary

Related issue #2700

[rokonec]

After discussion with Rainer we have decided to delete #ifdef VALIDATERESOURCESTRINGS sections as it has not been used since 2006

[benvillalobos]

Just making sure I understand this optimization at a high level.

1. TaskLoggingHelper uses BuildMessageEventArgs to log messages, and BuildMessageEventArgs is a LazyFormattedBuildEventArgs.
2. GetResourceString no longer auto-formats because TaskLoggingHelper ultimately uses a LazyFormattedBuildEventArgs
3. Any calls to TaskLoggingHelper.LogMessage that used to pass in a formatted string have been modified to pass in the resource strings unformatted, thus preventing an unnecessary format

If my understanding is correct, LGTM. I double checked and there shouldn't be any other calls to LogMessage that pass in a formatted string after this PR merged.

[rokonec]

@benvillalobos correct.

[rainersigwald]

Looks great, thanks!

[ladipro]

TaskLoggingHelper is part of public API surface and - strictly technically - the way it's refactored in this PR is not 100% safe. Say that someone has a derived class:

class MyLoggingHelper : TaskLoggingHelper
{
    public override string GetResourceMessage(string resourceName)
    {
        return "Hello: " + FormatResourceString(resourceName, null);
    }
}

Now with the change in this PR calling GetResourceMessage on such a class will crash with SO exception while previously it worked perfectly fine.

Am I being paranoid? Maybe.

Here's a less contrived example: Changing the formatting behavior can currently be achieved by overriding only FormatResourceString because then you get GetResourceMessage for free. With your change it is no longer true. Actually, case in point, check out TaskLoggingHelperExtension. This class would be broken exactly like so because GetResourceMessage wouldn't get the overriden behavior.

[rokonec]

@ladipro This is very good catch. I will fix it.

But truth to be told, the current implementation is counter intuitive - at least to me - as one would expect FormatResourceString calls GetResourceMessage and not vice versa, but since this 'silence' contract has been published, there is not much we can do about it as there could indeed be users relying on it - and as you have pointed out, we already relay on it in TaskLoggingHelperExtension.

[ladipro]

Thank you!

[rokonec]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).