Feature Request: Add ability to automatically hide metric columns if value is not set

[AvishaiDotan]

This is my first contribution to this repository.
I have read the contributing guidelines and hope this PR complies with them. If something is missing, I am more than happy to hear feedback. This is my first contribution, but I sincerely hope it will not be my last.

Technical:
I followed @timcassell instructions and replaced the returning boolean with an expression.
I also added sample tests for the repository reviewers to use during testing.

These samples do not need to be merged as this PR is relatively small. Their purpose is to make the reviewer's job easier.

[AvishaiDotan]

@dotnet-policy-service agree

[timcassell]

Please see comments on previous PRs for this issue. #2633 and #2658

[AvishaiDotan]

@timcassell
I reviewed my PR based on the issues you mentioned

My concern with this change is users adding the diagnoser and being surprised by no column showing. Perhaps the auto-hide feature should be behind a configuration on the diagnoser itself.

I have implemented a parameter-based hiding feature instead of using metric.Value > 0.

Previous code:
public bool GetIsAvailable(Metric metric) => metric.Value > 0;
Updated Code:
public bool GetIsAvailable(Metric metric) => Config.DisplayCompletedWorkItemCountWhenZero || metric.Value > 0;

To support this, I've implemented configuration files and parameter-based hiding, where the default is to always display the metric columns. For example:

I have also implemented a MetricDescriptorSingletonBasePattern and a MetricDescriptorConfigurationHandler to support injection of configuration within MetricDescriptor and to adhere to DRY principles.

Could you please provide a reference for situations where CompletedWorkItemCount and LockContentionCount benchmarks are not zero, so that I can create the tests?

[timcassell]

Could you please provide a reference for situations where CompletedWorkItemCount and LockContentionCount benchmarks are not zero, so that I can create the tests?

These are racy properties not really suitable for integration tests (they could be flaky). You could instead mock them. See SummaryTableTests and MarkdownExporterVerifyTests.

[AvishaiDotan]

@timcassell
I ignored the Singleton pattern and instead created a new instance of the descriptor with injected configs. Logically, it shouldn't affect the process, but I ran some tests to be sure, and I didn't find any impact.

My suggestion is to create a base class that implements this logic, instead of repeating the code

[timcassell]

My suggestion is to create a base class that implements this logic, instead of repeating the code

You can if you want, but I have a suspicion that it will have more code rather than less.

Btw, if you're going to do that, I would just inject the bool flag directly rather than the entire config object.

[AvishaiDotan]

My suggestion is to create a base class that implements this logic, instead of repeating the code

You can if you want, but I have a suspicion that it will have more code rather than less.

Btw, if you're going to do that, I would just inject the bool flag directly rather than the entire config object.

I’ve separated the configuration injection into its own logic. It’s not a huge amount of code, but I think it’s a smarter approach and will be easier to control. The getIsAvailable method stays the same. As for injecting the boolean, you know the project better than I do. My workaround was meant to let configurations affect the descriptors with multiple variables. But if that’s not really needed, injecting the boolean and skipping the base class for injection would probably be the better way to go.

[timcassell]

Sorry for the delay. I just tried out your changes to make sure it looks correct with a joined table with multiple configured , and it LGTM!

Btw, for future PRs, you should make a separate branch on your fork instead of working directly in the master branch. That way it will be easier to keep up to date with upstream and easier for others to pull your branch.