-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 Zero length check #2765
Fix Zero length check #2765
Conversation
Comments added in #2764; there is currently insufficient context as to whether there is an error here. The syntax used is not, by itself, either correct or incorrect. It is simply syntax, and has a current meaning, which this PR changes. Whether the original code, the proposed changed code, neither, or both; are correct - is a great question, but one that neither the issue nor the PR discusses. I suggest we start there. |
@@ -13,7 +13,7 @@ namespace StackExchange.Redis | |||
{ | |||
internal RedisKey(byte[]? keyPrefix, object? keyValue) | |||
{ | |||
KeyPrefix = keyPrefix?.Length == 0 ? null : keyPrefix; |
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.
Let's compare using:
using System;
Original(null);
Original("");
Original("abc");
Proposed(null);
Proposed("");
Proposed("abc");
static void Original(string? keyPrefix) => Write(keyPrefix?.Length == 0 ? null : keyPrefix);
static void Proposed(string? keyPrefix) => Write(keyPrefix == null || keyPrefix.Length == 0 ? null : keyPrefix);
static void Write(string? value)
=> Console.WriteLine(value is null ? "(null)" : "\"" + value + "\"");
outputs:
keyPrefix input | original output | proposed output |
---|---|---|
null | null | null |
"" | null | null |
"abc" | "abc" | "abc" |
So... this has no functional change
@@ -401,7 +401,7 @@ internal override bool AsBoolean() | |||
: Array.ConvertAll(_value, x => x.AsByteArray()!); | |||
|
|||
private bool IsSingleton => _value?.Length == 1; | |||
private bool IsEmpty => _value?.Length == 0; |
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.
this changes the value of IsEmpty
for null
values of _value
; currently a null
will return false
for IsEmpty
; with this change it will return true
. The question is what should IsEmpty
return for null
values? IMO we should retain the current behaviour, where null
reports false
here (note the separate IsNull
test). Changing the result of this property is a significant proposition that would require justification.
Closing this; it appears to be part of a bulk creation of similar PRs on the assumption that |
Fix #2764