-
-
Notifications
You must be signed in to change notification settings - Fork 945
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
Fix ParamColumn null representation #2122
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.