-
-
Notifications
You must be signed in to change notification settings - Fork 375
C# 8 phase 1 #472
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
C# 8 phase 1 #472
Conversation
vslee
commented
Oct 25, 2019
- jjxtra began by marking some objects using the nullable (?) reference type signifier
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 like a good start into moving this codebase to cs8 😃
{ | ||
throw new InvalidOperationException("Invalid salt"); | ||
} | ||
var cs = new CryptoStream(encrypted, AES.CreateDecryptor(), CryptoStreamMode.Read); |
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.
Maybe here would be good to use using var
expression.
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.
Can you add an example of what you mean? Then I can add a commit to this PR. Alternatively, I can merge this PR and you can put it into your current PR or a new one.
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 was referring to the same expression in the second example of this post.
Also, I can add it later :)
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.
Good start
Need @jjxtra to put an approving review before I can merge (I know it's strange for him to be reviewing his own code but since I was the one starting the PR, I can't review my own PR) |
I don't know if I approve of my code to be honest :) We might consider backing out of nullable reference types on a project level and going to a per file level to reduce the warning barrage. |
- we can go back to project wide once we convert everything
Far fewer warnings now (~50 instead of ~500) |
I think we can merge now, and then whittle the rest of the warnings down with time. |
progress towards #450 |