Skip to content

Conversation

@workgroupengineering
Copy link
Contributor

@workgroupengineering workgroupengineering commented May 7, 2021

fixes #1700

This PR

  • Don't contain the break change
  • It has an integration test

@workgroupengineering workgroupengineering force-pushed the Bugfix/Issue_1700 branch 3 times, most recently from c2d6400 to 3ee9743 Compare May 17, 2021 16:21
@workgroupengineering workgroupengineering marked this pull request as ready for review May 17, 2021 16:22
Copy link
Member

@AndreyAkinshin AndreyAkinshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the logging approach is the best way to report a warning in this case. In most cases, BenchmarkDotNet users read only the bottom part of the log (with the summary table) and don't scroll up. Thus, this warning will be most likely lost.
On the other hand, we could define IAnalyser which reports such a warning. The IAnalyser approach has two advantages:

  1. It prints all the warning below the summary table (so the warnings are more visible for the users)
  2. The list of the warnings could be fetched programmatically (so it's possible to introduce an assertion that the benchmarks don't produce any warnings).

However, this particular case is tricky because we have to report warning on the stage of config building (not at the final stage of the BenchmarkReport analysis). I guess, we could introduce an IReadOnlyList<Conclusion> property in IConfig and use it to report issues related to malformed config in a structured way.

@adamsitnik what do you think?

@workgroupengineering
Copy link
Contributor Author

Please @AndreyAkinshin can you review? If the proposed changes are not to your liking. Please close this PR. 😄

@AndreyAkinshin
Copy link
Member

Closed since #1796 looks better.

@workgroupengineering workgroupengineering deleted the Bugfix/Issue_1700 branch December 15, 2021 16:01
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

2 participants