Skip to content

Conversation

@YegorStepanov
Copy link
Contributor

@YegorStepanov YegorStepanov commented Oct 17, 2022

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 fixes(configuration): Not unique exporter for exporter type  #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 YegorStepanov marked this pull request as draft October 17, 2022 16:44
@YegorStepanov YegorStepanov marked this pull request as ready for review October 17, 2022 17:04
@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Oct 17, 2022

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
Copy link
Contributor Author

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?

@workgroupengineering
Copy link
Contributor

workgroupengineering commented Oct 18, 2022

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.

@YegorStepanov
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@workgroupengineering
Copy link
Contributor

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.

@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Oct 18, 2022

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" 🤔

@delixfe
Copy link

delixfe commented Mar 13, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Not unique exporter for exporter type

3 participants