BenchmarkDotNet icon indicating copy to clipboard operation
BenchmarkDotNet copied to clipboard

Rework "Not unique exporter for exporter type" PR

Open YegorStepanov opened this issue 3 years ago • 8 comments
trafficstars

Fixes #1700

Problem

Starting with #1796 PR (#1700 issue), only one exporter per type can be.

This means that the following cases stopped working in 0.13.2:

// DefaultConfig has MarkdownExporterAttribute.Default already
// It prints a warning and Atlassian file is not created.
[MarkdownExporterAttribute.Atlassian] 
public class Klass
{
    [Benchmark] public void M() { }
}

Also, it means that it will never works:

// not working for ManualConfig too
[XmlExporterAttribute.Full] // it uses XmlExporter.Full under the hood
[XmlExporterAttribute.Brief] // it uses XmlExporter.Brief under the hood

Solution

  1. revert #1796
  2. compare by IExporter.Name
  3. rename IExporter.Name -> IExporter.Id because Name is misleading, the other entities in BDN uses Id for it:
Characteristic.Id
IAnalyser.Id
ILogger.Id
IDiagnoser.Ids
  1. Add RPlotValidator to check that user do not override csv separator (Currently, all version of csv generates file with same name)

YegorStepanov avatar Oct 17 '22 15:10 YegorStepanov

These benchmarks are manually checked:

Click me
[DryJob, MarkdownExporterAttribute.Atlassian]
public class MyTest1
{
    [Benchmark] public void MyMethod() { }
}

// validator error
[CsvMeasurementsExporter(CsvSeparator.Semicolon)]
[RPlotExporter]
[DryJob]
public class MyTest2
{
    [Benchmark] public void MyMethod() { }
}

[DryJob]
[MarkdownExporterAttribute.Atlassian]
[XmlExporterAttribute.Full]
[XmlExporterAttribute.Brief]
public class MyTest3
{
    [Benchmark] public void MyMethod() { }
}

[DryJob]
[CustomExporter("Benchmark")]
public class MyTest4
{
    [Benchmark] public void MyMethod() { }
}

public class CustomExporterAttribute : ExporterConfigBaseAttribute
{
    public CustomExporterAttribute(string title) : base(new CustomExporter(title)) { }
}

public class CustomExporter : IExporter
{
    private readonly string _title;
    public CustomExporter(string title) => _title = title;
    public string Id => GetType().Name;

    public IEnumerable<string> ExportToFiles(Summary summary, ILogger consoleLogger)
    {
        Console.WriteLine($"{Id} - {_title}");
        yield break;
    }

    public void ExportToLog(Summary summary, ILogger logger) { }
}

YegorStepanov avatar Oct 17 '22 17:10 YegorStepanov

I'm not at all sure about renaming (IExporter.Name -> IExporter.Id)

I don't think anyone uses it so it one line change for customers.

We will never be able to use "C# default interface implementation" due to .NET Framework?

YegorStepanov avatar Oct 17 '22 22:10 YegorStepanov

I am not very convinced of the solution. Because if you have developed custom exporters, no anomalies will be reported unless a validator is written.

The original issue was not written to solve the RPlotExporter problem but to prevent problems like this from happening in the future.

workgroupengineering avatar Oct 18 '22 12:10 workgroupengineering

I am not very convinced of the solution. Because if you have developed custom exporters, no anomalies will be reported unless a validator is written.

@workgroupengineering Could you write an example please? It covers your issue (#1700)

If you mean about 'IExporterDependencies':

  1. only RPlotExporter uses it.
  2. it's internal interface.

If the plots are rewritten in C#, IExporterDependencies will be deleted.

YegorStepanov avatar Oct 18 '22 13:10 YegorStepanov

Indeed, it would be better to remove IExporterDependencies now. And pass separator to R. But active contributors don't know R.

YegorStepanov avatar Oct 18 '22 13:10 YegorStepanov

I encountered problem #1700 during development a PR which allowed append result to csv artifact. The artifact was overwritten.

Indeed, it would be better to remove IExporterDependencies now. And pass separator to R. But active contributors don't know R.

Probably is better solution remove IExporterDependencies from RPlotExporter.

I would keep IExporterDependencies and would make it public it would allow you to easily extend it starting from an artifact as RPlotExporter already does.

workgroupengineering avatar Oct 18 '22 14:10 workgroupengineering

Instead of making IExporterDependencies public, what do you think about the next one:

The naming of Json/Xml exporters is depending on "exporter style":

XmlExporter.Full -> creates "NAME-full.xml"
XmlExporter.Brief -> creates "NAME-brief.xml"
JsonExporter.Full -> creates "NAME-full.json"
JsonExporter.Brief -> creates "NAME-brief.json"

Maybe we should do the same for csv?

CsvExporter(CsvSeparator.Comma) -> creates "NAME-comma.csv"
CsvExporter(CsvSeparator.Semicolon) -> creates "NAME-semicolon.csv"
CsvExporter(CsvSeparator.CurrentCulture) -> creates "NAME-comma.csv" or creates "NAME-semicolon.csv"

CsvSeparator.CurrentCulture looks dangerous, but as I understand it, R graphs depend on this

[Edit]

CsvExporter(CsvSeparator.CurrentCulture) -> can create "NAME-currentculture.csv" 🤔

YegorStepanov avatar Oct 18 '22 15:10 YegorStepanov

@YegorStepanov, please don't forget individual names for the Markdown exporters.

delixfe avatar Mar 13 '23 16:03 delixfe