Validate IQueryable<> is valid
#4580
Conversation
viceroypenguin
requested review from
igor-tkachev,
sdanyliv,
MaceWindu,
jods4 and
Shane32
as code owners
|
/azp run test-all |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I do not see reason why you have added this validation. In 95% it is not needed. Just build tree. Validation needed only for Async methods.
|
|
Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed. |
|
@sdanyliv we discussed it before - if we validate correctness of call for async, it will be strange to not do it for sync apis |
|
/azp run test-all |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run test-all |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -252,7 +253,7 @@ public static IEnumerable<TSource> DeleteWithOutput<TSource>(this IQueryable<TSo | |||
| /// <item>SQL Server 2005+</item> | |||
| /// </list> | |||
| /// </remarks> | |||
| public static Task<int> DeleteWithOutputIntoAsync<TSource,TOutput>( | |||
| public static async Task<int> DeleteWithOutputIntoAsync<TSource,TOutput>( | |||
There was a problem hiding this comment.
I know Microsoft recommends to add unnecessary async contexts for better stack trace, but I don't think we should do it.
There was a problem hiding this comment.
I'll disagree with you, but I'm good with it either way.
There was a problem hiding this comment.
Well:
- we don't use this approach anywhere else
- it goes in different direction with Possible Feature/Enhancement: Pooling Tasks? #3437. But personally I don't expect changes like that could be an improvement for such heavy-weight task as ORM
There was a problem hiding this comment.
- Valid, then yeah, it's worth having a conversation at some point.
- For an ORM, where a majority of the time is gonna be db related, and heavy allocations due to loading objects, I'm less concerned about the allocation of a
Task, and more concerned about having accurate stack traces.
|
/azp run test-all |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Part of #4475