-
-
Notifications
You must be signed in to change notification settings - Fork 995
Hiding columns #1890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hiding columns #1890
Conversation
Thanks, I like the API 👍 Just one suggestion: maybe instead of an enum, what would you think about a class with column names as constants? public static class ColumnNames
{
public const string Namespace = "Namespace";
public const string Type = "Type";
// ...
} |
Yeah, don't, it's depressing :) Thanks for working on this. |
@ltrzesniewski It looks better then enum and no compatibility issues. I started with flags enums, confused OR with AND :) and I don't like that they look different: [HideColumns("Mean", "Error")]
[HideColumns(Column.Mean | Column.Error)] If we add all columns to the enum/class, we can add [AddColumns] that will implicitly adds diagnostics: [AddColumns(Column.P90, Column.Allocated)] //adds [MemoryDiagnostic] and only Allocated, I suppose I'm not sure if someone needs it [Upd] Thanks for the idea! the user will be able to pass his column instance to the attribute. |
I wanted to do something like this: public static class Column
{
public static readonly IColumn Mean; //const not working too
}
[HideColumns(Column.Mean)] but attribute restrictions are very painful: dotnet/roslyn#16898 (comment) @ltrzesniewski suggestion to compare only by @adamsitnik @AndreyAkinshin What is your opinion on this? If enum is the best option, should I add all possible columns? The total number of columns is ~100, but most are useful when they appears. |
Nice! I sometimes have What about an [Edit] I also like @ltrzesniewski's idea for const string names instead of enums, which would play nicely with an |
Maybe add
It will greatly simplify PR. |
I suppose a |
ConfidenceIntervalErrorColumnAttribute(ConfidenceLevel level = ConfidenceLevel.L999)
RankColumnAttribute(NumeralSystem system = NumeralSystem.Arabic)
StatisticalTestColumnAttribute(...) // 3 ctors Almost every in Mutators folder: for example: GcForceAttribute(bool value = true)
GcServerAttribute(bool value = false)
InvocationCountAttribute(int invocationCount, int unrollFactor = 1)
MaxRelativeErrorAttribute(double maxRelativeError) |
This is how I'm currently doing it for Job columns: It's not attribute-based, but relatively simple. And it would be easy to add command line support. |
3b9cd31
to
5bac031
Compare
Moved to const strings. I updated PR description. I hid tests/docs/samples to increase probability of code review. Any other suggestions? @timcassell Did you mean column adding, not showing? @mawosoft I've read your code before writing this. Thanks :) |
Yes, that's what I meant. :)
|
@timcassell I extend Summary interface by IColumnHidingRule, so if in the future there is a probability of adding AddColumns, then I should do it in another way. |
You could change it to |
@timcassell IColumnHidingRule.NeedToHide() forces hiding, not On/Off |
Oh, so you just force hide, but you can't force show. That makes sense. Yeah, I think that's fine how it is. |
One drawback: IntelliSense doesn't suggest |
…mpletion; StatisticalTest/HardwareCounters/CiLower are missing
When all columns are disabled, it was printing: | | | Replace it with the message, which is in contrast with errors
…Mean&Error are hidden
…tioColumn.RatioMean
5bac031
to
82d7b2f
Compare
Added CLI support: The column started with dash( Example: |
Instead of extending IConfig by smth like internal class ColumnFilter : IFilter
{
public bool Predicate(BenchmarkCase benchmarkCase) => true;
public bool NeedToHide() => ...;
} @adamsitnik, what do you advise to do? |
# Conflicts: # src/BenchmarkDotNet/Configs/ManualConfig.cs # src/BenchmarkDotNet/ConsoleArguments/ConfigParser.cs
- don't store Names in the field when there is no need to - make it possible to apply `[HideColumnsAttribute]` to entire assembly - add comment explaining why Column class is public - make two existing column hiding rules public so they can be reused - Column.IsCommon is better name than Column.IsCommonColumn - simplify LINQ -add sample
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, big thanks for your contribution @YegorStepanov !
closes #1775 closes #299; partially closes #1603 (there is an open PR for this issue)
It affects Console, Markdowns, Html exporters only.
API
Additionally, you can implement custom rule:
When using
--join
these attributes add up:Instead hiding it moves column value to the rows above the table when:
@stephentoub @timcassell @paulomorgado @ltrzesniewski Hello everyone! You have liked the issue. Any thoughts about proposed API? Is it covers all yours cases? Maybe I forgot to include some columns to the enum?
@stephentoub I'm afraid to imagine how much time you spent deleting columns...
TODO
--hide Mean Error
,