-
Notifications
You must be signed in to change notification settings - Fork 262
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
Test | Code syntax improvements for TVP test #1374
Conversation
96b9f2d
to
b4b990d
Compare
using SqlDataReader dr = cmd.ExecuteReader(); | ||
dr.Read(); | ||
VerifyReaderTypeAndValue("Test Simple Parameter [Data Type]", expectedBaseTypeName, expectedTypeName, dr[0], expectedTypeName, paramValue); | ||
dr.Dispose(); |
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.
Sometimes using this new syntax could add complexity to code. At least, I'd rather have this section with braces.
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 definitely agree. It was in the code from before to call Dispose. I was planning to change that in the next PR, but since you mentioned that I will do it in this PR.
using SqlDataReader dr = cmd.ExecuteReader(); | ||
dr.Read(); | ||
VerifyReaderTypeAndValue("Test Simple Parameter [Variant Type]", "SqlDbType.Variant", dr, expectedTypeName, expectedBaseTypeName, paramValue); | ||
dr.Dispose(); |
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.
Same comment here and for the similar sections.
cmd2.ExecuteNonQuery(); | ||
|
||
cmd2.CommandText = string.Format("SELECT f1 FROM {0}", OutputTableName); | ||
; |
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.
; |
{ | ||
dr.Read(); | ||
VerifyReaderTypeAndValue("Test SqlDataReader TVP [Data Type]", expectedBaseTypeName, expectedTypeName, dr[0], expectedTypeName, paramValue); | ||
dr.Dispose(); |
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.
We expect Dispose()
to be called automatically while we use the using
block. So, this function call is not necessary.
This could be applied to similar cases.
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.
Generally, I'd rather to keep the type name following new
keyword unless the type name was too long.
|
||
Random r = new Random(8888); | ||
private readonly int _errorPos = 0; | ||
readonly Random _r = new(8888); |
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.
readonly Random _r = new(8888); | |
private readonly Random _r = new(8888); |
cmd = new SqlCommand(GetProcName(tvpPerm)) | ||
{ | ||
CommandType = CommandType.StoredProcedure | ||
}; |
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 don't see any benefit on some changes like this in the tests. It's only added more lines.
This PR is just to address C# 9 related changes in TVP test. This will make the following PRs easier to follow.