-
Notifications
You must be signed in to change notification settings - Fork 5
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
#34 nullable c# 8 #39
Conversation
… with nullable reference type checks enabled.
…able reference types.
public static ConnectionCredentials ToTwitchLib(this TwitchCredentials credentials, IDataProtection dataProtection) | ||
{ | ||
if (credentials.AuthToken == null) | ||
throw new System.ArgumentException("Unable to convert null AuthToken", nameof(credentials)); |
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.
Assumed an exception was better here than just letting the decrypt blow up on an empty byte[]
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.
Agreed.
@@ -45,25 +45,25 @@ public void Disconnect() | |||
throw new NotImplementedException(); | |||
} | |||
|
|||
protected virtual void Client_OnLog(object sender, OnLogArgs e) | |||
protected virtual void Client_OnLog(object? sender, OnLogArgs e) |
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.
All these sender args need to be nullable since twitchlib is not c#8
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.
*yet
I'll ping LuckyNoS7evin to see if his restructure is going to change that.
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.
Some additional notes inline
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's a solid first pass to get nullable reference feature enabled.
public static ConnectionCredentials ToTwitchLib(this TwitchCredentials credentials, IDataProtection dataProtection) | ||
{ | ||
if (credentials.AuthToken == null) | ||
throw new System.ArgumentException("Unable to convert null AuthToken", nameof(credentials)); |
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.
Agreed.
@@ -45,25 +45,25 @@ public void Disconnect() | |||
throw new NotImplementedException(); | |||
} | |||
|
|||
protected virtual void Client_OnLog(object sender, OnLogArgs e) | |||
protected virtual void Client_OnLog(object? sender, OnLogArgs e) |
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.
*yet
I'll ping LuckyNoS7evin to see if his restructure is going to change that.
Closes #34
Please note this is an initial "surgery with a chainsaw" change to get nullable reference types in place with all the code still compiling and the tests working.
While I'm certain some of the chosen defaults could be improved, and there should probably be some unit tests added to drive out such scenarios, the intent here is to have this feature enabled sooner rather than later so that the problem doesn't grow worse in the meantime.