Fix ParamColumn null representation #2122
Conversation
| @@ -10,7 +10,8 @@ namespace BenchmarkDotNet.Parameters | |||
| { | |||
| public class ParameterInstance : IDisposable | |||
| { | |||
| public const string NullParameterTextRepresentation = "?"; | |||
| public const string NullParameterTextRepresentation = "Null"; | |||
There was a problem hiding this comment.
How about "<null>"? Makes it more obvious that it's actual null and not the string value "Null".
There was a problem hiding this comment.
It better to make it in lowercase Null -> null.
<null>
I like any option except ?.
Although the string value still can be \<null\>.
In addition, we can use a different color for special values (only suitable for the console output)
There was a problem hiding this comment.
Although the string value still can be
\<null\>.
I don't think it needs to be escaped. The XML serializer will automatically escape those to < >. Unless we don't want those escaped in XML, then maybe (null) instead.
There was a problem hiding this comment.
Sorry, github markdown doesn't like <> characters and removes them if they are not escaped. I forgot to remove escaping when I added the single quotes.
There was a problem hiding this comment.
Looks a little odd to me.
Method | ParamProperty | Mean | Error | StdDev |
---------- |-------------- |---------:|--------:|--------:|
Benchmark | <null> | 102.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | False | 202.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | True | 302.0 ns | 6.09 ns | 1.58 ns | ^
Method | ParamProperty | Mean | Error | StdDev |
---------- |-------------- |---------:|--------:|--------:|
Benchmark | <null> | 102.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | A | 202.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | B | 302.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | C | 402.0 ns | 6.09 ns | 1.58 ns | ^
Method | ParamProperty | Mean | Error | StdDev |
---------- |-------------- |---------:|--------:|--------:|
Benchmark | <null> | 102.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | 0 | 202.0 ns | 6.09 ns | 1.58 ns | ^
There was a problem hiding this comment.
I think it looks good. But I guess that's just a matter of preference.
[Edit] But I think I like (null) better.
Method | ParamProperty | Mean | Error | StdDev |
---------- |-------------- |---------:|--------:|--------:|
Benchmark | (null) | 102.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | False | 202.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | True | 302.0 ns | 6.09 ns | 1.58 ns | ^
Method | ParamProperty | Mean | Error | StdDev |
---------- |-------------- |---------:|--------:|--------:|
Benchmark | (null) | 102.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | A | 202.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | B | 302.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | C | 402.0 ns | 6.09 ns | 1.58 ns | ^
Method | ParamProperty | Mean | Error | StdDev |
---------- |-------------- |---------:|--------:|--------:|
Benchmark | (null) | 102.0 ns | 6.09 ns | 1.58 ns | ^
Benchmark | 0 | 202.0 ns | 6.09 ns | 1.58 ns | ^
Null value is ok, but it is shown as unknown.
It's impossible to get Unknown value now, but I decided to add handling for this situation.