Fix #1715

[YegorStepanov]

trafficstars

fix #1715

[AndreyAkinshin]

I'm OK with removing quotes, but it's a breaking change that can cause problems for BenchmarkDotNet users who parse the output of Exporters. If we introduce this change, we should provide a way to rollback to the previous behavior.

[YegorStepanov]

Done.

Instead of extending SummaryStyle, I was thinking of adding an assembly attribute:

[assembly: RevertOldBehaviourForBenchmarkName]

The attribute seems more elegant for this.

[YegorStepanov]

[assembly: RevertOldBehaviourForBenchmarkName]

Option 1

it will work like this:

//BenchmarkConverter.cs
//type is benchmark type
BenchmarkRunInfo MethodsToBenchmarksWithFullConfig(Type type, ...)
{
    bool revertOldBehaviour = type.Assembly.GetCustomAttributes().OfType<RevertOldBehaviourForBenchmarkName>();
    var targets = GetTargets(..., revertOldBehaviour);
}

cons:

• benchmarks can be defined in different assemblies.
• hard to test it

[Edit] It looks we can get all assemblies with AppDomain.CurrentDomain.GetAssemblies() and cache the result. So, the option1 make sense too (to avoid polluting SummaryStyle).

Option 2

Instead, we can make SummaryStyle.OldBehaviorForBenchmarkName internal and only allow it to set via an assembly attribute.

cons

• hard to test (we can simply remove RevertOldBehaviorTest)

Changelog

Maybe it makes sense to add a "breaking change" paragraph to the changelog?

Users will notice this breaking change when they break their CI or erase benchmark history (because the name is changed).

The BreakingChange.md will also be useful. I can help with it (also for few older version).

[Edit2] There is BDN.IntegrationTests.ConfigPerAssembly project, so we can test the assembly attribute.

[AndreyAkinshin]

1. It would be better to use a more meaningful option name instead of oldBehaviorForBenchmarkName (you never know how many behavior generations we will have in the future). E.g., QuoteBenchmarkNamesWithSpecialChars.
2. We need a flexible way to control this behavior. The approach with assembly attributes is not flexible enough, it can be inconvenient for some users. Meanwhile, I don't really want to extend the Config API (which is already quite extensive) with additional temporary properties.

I was thinking about an approach inspired by the .NET Runtime knobs. We can introduce string? IConfig.GetKnob(string key) that returns the value of the given knob (additionally, we can introduce extension methods like bool? IConfig.GetKnobBool(string key) which implements the bool parsing logic). The Knob can be specified via ManualConfig (e.g., ManualConfig.WithKnob(string key, string value)), an attribute ([Knob(string key, string value)]), or via environment variables (e.g., BDN_<KnobKey>).

@adamsitnik @YegorStepanov what do you think about such a design?

[YegorStepanov]

I like it. Will be done.

Does it make sense to add the Knob class?

public class Knob
{
     public const string QuoteBenchmarkNamesWithSpecialChars = "...";
}

[Knob(Knob.QuoteBenchmarkNamesWithSpecialChars, true)]

[AndreyAkinshin]

@YegorStepanov, I was thinking about something like this:

public abstract class Knob<T>
{
    public string Key { get; }
    public T DefaultValue { get; }

    protected Knob(string key, T defaultValue)
    {
        Key = key;
        DefaultValue = defaultValue;
    }

    protected abstract T Parse(string value);

    public T GetValue(IConfig config)
    {
        string value = config.GetKnobValue(Key) ?? Environment.GetEnvironmentVariable("BDN_" + Key);
        return Parse(value);
    }
}

public class BoolKnob : Knob<bool>
{
    public BoolKnob(string key, bool defaultValue) : base(key, defaultValue) { }

    protected override bool Parse(string value) =>
        value?.ToLowerInvariant() switch
        {
            "0" or "f" or "false" => false,
            "1" or "t" or "true" => true,
            _ => DefaultValue
        };
}

public class Knobs
{
    public const string QuoteBenchmarkNamesWithSpecialCharsKey = "QuoteBenchmarkNamesWithSpecialChars";

    public BoolKnob QuoteBenchmarkNamesWithSpecialChars = new (QuoteBenchmarkNamesWithSpecialCharsKey, false);
}

P.S. Let's wait for an opinion by @adamsitnik before we start to implement this.