Skip to content
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

Add template methods around DbConnection inside RelationalConnection #22849

Merged
merged 7 commits into from
Oct 5, 2020

Conversation

joaopgrassi
Copy link
Member

Resolves #21327

@joaopgrassi joaopgrassi marked this pull request as draft September 30, 2020 08:20
@joaopgrassi
Copy link
Member Author

joaopgrassi commented Sep 30, 2020

There's a test failing (RelationalApiConsistencyTest) due to the newly added template method for DbConnection.CloseAsync()... Not sure how to proceed. The method is protected, so I can't add to the exclusion list (AsyncMethodExceptions). If I make it public then it's not consistent with the rest of the template methods.. and just adding the cancellation token to make the tests pass seems hacky (since it's not used by DbConnection.CloseAsync). Any ideas?

@@ -227,11 +227,19 @@ public virtual void EnlistTransaction(Transaction transaction)
Dependencies.TransactionLogger.ExplicitTransactionEnlisted(this, transaction);
}

DbConnection.EnlistTransaction(transaction);
ConnectionEnlistTransaction(transaction);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dotnet/efteam Any thoughts on naming these template methods before merging? (We will review in API review later anyway.)

@ajcvickers
Copy link
Member

@joaopgrassi You should be able to add the method to AsyncMethodExceptions even though it is protected. Something like:

typeof(RelationalConnection).GetMethod("CreateDbConnection", BindingFlags.Instance | BindingFlags.NonPublic);

@joaopgrassi
Copy link
Member Author

joaopgrassi commented Sep 30, 2020

typeof(RelationalConnection).GetMethod("CreateDbConnection", BindingFlags.Instance | BindingFlags.NonPublic);

Right 😅. Was so deep into the errors forgot the obvious, lol.

I did notice though that some methods return a ValueTask but they were not being caught by this consistency check. Looking more I saw that's because inside Async_methods_should_have_overload_with_cancellation_token_and_end_with_async_suffix it just looks for Task. Not sure if it's something to change as well.. probably not now since it will produce errors everywhere.

@joaopgrassi joaopgrassi marked this pull request as ready for review October 1, 2020 07:33
@ajcvickers ajcvickers merged commit b715a35 into dotnet:main Oct 5, 2020
@ajcvickers
Copy link
Member

@joaopgrassi Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding template methods for DbConnection calls
2 participants