chore: Enable nullable annotation for validators #2902
Conversation
| // to have backward compatibility for people who implemented IValidator(Benchmark[] benchmarks) | ||
| public static implicit operator ValidationParameters(BenchmarkCase[] benchmarksCase) => new ValidationParameters(benchmarksCase, null); | ||
| public static implicit operator ValidationParameters(BenchmarkRunInfo benchmarkRunInfo) => new ValidationParameters(benchmarkRunInfo.BenchmarksCases, null); | ||
| // Note: Following implicit operators are expected to be used for test projects. |
There was a problem hiding this comment.
Remove backward compatibility comments.
Because when null is passed to config parameter.
It'll throw NullReferenceException when accessing config property.
These implicit operators seems to be used for test projects.
filzrev
force-pushed
the
chore-enable-nullable-3
branch
from
e0b5671 to
8f4bfa3
Compare
|
Please re-run |
| public IReadOnlyList<BenchmarkCase> Benchmarks { get; } | ||
|
|
||
| public ImmutableConfig? Config { get; } | ||
| public ImmutableConfig Config { get; } |
There was a problem hiding this comment.
I don't quite understand why this is changed from nullable when null is explicitly passed in the implicit operators. We should just remove those operators if they are no longer useful.
There was a problem hiding this comment.
We should just remove those operators if they are no longer useful.
I think that's the correct way to fix it.
Though when removing these implicit operators. It cause about 39 build errors on test projects. and without implicit operators.
It need to add a lot of additional code. (e.g. Create ToValidationParameters extension method and call this method on every location that using implicit operators)
There was a problem hiding this comment.
What about changing null to DefaultConfig.Instance.CreateImmutableConfig() for now?
There was a problem hiding this comment.
I've modified code to use DefaultConfig.Instance.CreateImmutableConfig() on implicit operators that accept BenchmarkCase[] benchmarksCase as parameter.
And use actual config when accept (BenchmarkRunInfo benchmarkRunInfo) as parameter.
In anyway, this config is not be referenced by existing unit tests/integration tests.
filzrev
force-pushed
the
chore-enable-nullable-3
branch
from
5bacc38 to
d5020ae
Compare
2 participants
This PR intended to fix a part issue #2815