Skip to content
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

Merged
merged 13 commits into from Aug 25, 2022
Merged

Hiding columns #1890

merged 13 commits into from Aug 25, 2022

Conversation

YegorStepanov
Copy link
Contributor

@YegorStepanov YegorStepanov commented Jan 11, 2022

closes #1775 closes #299; partially closes #1603 (there is an open PR for this issue)

It affects Console, Markdowns, Html exporters only.

API

[HideColumns(Column.Mean, "Error")] // Hides all columns with such names.
class Benchmarks { ... }

//by config:
DefaultConfig.Instance
    .HideColumns(Column.Mean, "Error")
    .HideColumns(new MyColumn()); //compare by Id, not by name

Additionally, you can implement custom rule:

public class MyRule : IColumnHidingRule
{
    public bool NeedToHide(IColumn column) => column.Category == ColumnCategory.Statistics;
}

DefaultConfig.Instance.HideColumns(new MyRule());

When using --join these attributes add up:

//both columns are hidden when --join or RunAllJoined()
[HideColumns("Column1")] class Benchmarks1 {}
[HideColumns("Column2")] class Benchmarks2 {}

Instead hiding it moves column value to the rows above the table when:

  • one value
  • all values are the same

@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

  • cli support: --hide Mean Error,
  • code completion to the cli (waiting for migration to System.CommandLine)

@ltrzesniewski
Copy link
Contributor

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";
    // ...
}

@stephentoub
Copy link
Member

@stephentoub I'm afraid to imagine how much time you spent deleting columns...

Yeah, don't, it's depressing :) Thanks for working on this.

@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Jan 11, 2022

@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.

@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Jan 12, 2022

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 ColumnName, so [HideColumns(ColumnNames.Mean)] will hide all columns named "Mean".

@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.

image

@timcassell
Copy link
Collaborator

timcassell commented Jan 12, 2022

Nice! I sometimes have UnrollFactor column appear when I don't care about it.

What about an IncludeOnlyColumns or just OnlyColumns option that excludes all others except the ones you specify?

[Edit] I also like @ltrzesniewski's idea for const string names instead of enums, which would play nicely with an OnlyColumns as well when you want to also include custom columns (like the names of params).

@YegorStepanov
Copy link
Contributor Author

YegorStepanov commented Jan 12, 2022

What about an IncludeOnlyColumns or just OnlyColumns option that excludes all others except the ones you specify?

Maybe add NoColumns/Empty/BlankConfig for it? It's like DefaultConfig but without any columns.
It would duplicate API, some column attributes have additional settings, that can't be set by [OnlyColumns]

[Edit] I also like @ltrzesniewski's idea for const string names instead of enums, which would play nicely with an OnlyColumns as well when you want to also include custom columns (like the names of params).

It will greatly simplify PR.

@timcassell
Copy link
Collaborator

timcassell commented Jan 12, 2022

Maybe add NoColumns/Empty/BlankConfig for it? It's like DefaultConfig but without any columns. It would duplicate API, some column attributes have additional settings, that can't be set by [OnlyColumns]

I suppose a HideAllColumns plus AddColumns would do the trick. Perhaps the HideAllColumns could have an option to still show the ones with special settings? I'm not really sure how that should work... What's an example of one of the columns with special settings where this wouldn't work?

@YegorStepanov
Copy link
Contributor Author

ConfidenceIntervalErrorColumnAttribute(ConfidenceLevel level = ConfidenceLevel.L999)
RankColumnAttribute(NumeralSystem system = NumeralSystem.Arabic)
StatisticalTestColumnAttribute(...) // 3 ctors 

Almost every in Mutators folder:

image

for example:

GcForceAttribute(bool value = true)
GcServerAttribute(bool value = false)
InvocationCountAttribute(int invocationCount, int unrollFactor = 1)
MaxRelativeErrorAttribute(double maxRelativeError)

@mawosoft
Copy link
Contributor

This is how I'm currently doing it for Job columns:
https://mawosoft.github.io/Mawosoft.Extensions.BenchmarkDotNet/api/Mawosoft.Extensions.BenchmarkDotNet.JobColumnSelectionProvider.html

It's not attribute-based, but relatively simple. And it would be easy to add command line support.

@YegorStepanov
Copy link
Contributor Author

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 :)

@timcassell
Copy link
Collaborator

@timcassell Did you mean column adding, not showing?

Yes, that's what I meant. :)

AddColumns or OnlyColumns isn't as big of a deal as HideColumns I think, and they can always be added in a separate PR if there's enough need for it.

@YegorStepanov
Copy link
Contributor Author

@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.

@timcassell
Copy link
Collaborator

You could change it to IColumnVisibilityRule. I think adding would just be negating whatever you have for hiding anyway, right?

@YegorStepanov
Copy link
Contributor Author

@timcassell IColumnHidingRule.NeedToHide() forces hiding, not On/Off

@timcassell
Copy link
Collaborator

Oh, so you just force hide, but you can't force show. That makes sense. Yeah, I think that's fine how it is.

@YegorStepanov
Copy link
Contributor Author

One drawback: IntelliSense doesn't suggest Column.Mean when writing Mean, but did it for enum

@YegorStepanov
Copy link
Contributor Author

Added CLI support: -h or --hide

The column started with dash(-) treated like a new command (the double quotes don't help), but the dash can be used in the middle of name: my-column-name

Example: --hide Mean Error "Method" "Alloc Ratio"

@YegorStepanov
Copy link
Contributor Author

Default, no rules:

image

--hide Method Mean Error "Alloc Ratio"

image

--hide Method Mean Error "Alloc Ratio" Ratio Allocated

image

@YegorStepanov
Copy link
Contributor Author

Instead of extending IConfig by smth like IConfig.GetColumnHidingRules(), it is possible to pass the data through a magic filter (and downcasting it when needed)

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
@adamsitnik adamsitnik added this to the v0.13.2 milestone Aug 25, 2022
Copy link
Member

@adamsitnik adamsitnik left a 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 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants