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

Issue #1239 resolution - QueryAsync missing buffered parameter #1882

Closed
wants to merge 4 commits into from
Closed

Issue #1239 resolution - QueryAsync missing buffered parameter #1882

wants to merge 4 commits into from

Conversation

SoftStoneDevelop
Copy link

@SoftStoneDevelop SoftStoneDevelop commented Feb 6, 2023

This change resolves #1239.

I had to switch to net48;netstandard2.1 because of the Microsoft.Bcl.AsyncInterfaces and System.Linq.Async packages, but I think it's worth it.

@SoftStoneDevelop
Copy link
Author

It would be nice to add an asynchronous connection close to the method:

private static async IAsyncEnumerable<T> QueryAsync<T>(this IDbConnection cnn, Type effectiveType, CommandDefinition command)

But I didn’t want to do something like TryOpenAsync, so I made a proposal in .NET to create an IAsyncDbConnection interface:
dotnet/runtime#81723

@mgravell
Copy link
Member

mgravell commented Jun 9, 2023

Right; so: I don't think we can take this as-is; there's way too much API breakage here, and honestly, taking buffered as a parameter here is just a bad choice, as buffered and unbuffered are fundamentally incompatible shapes

IMO the correct change here is just the addition of a single API:

public static IAsyncEnumerable<T> QueryUnbufferedAsync<T>(this DbConnection cnn, string sql, object param = null,
    DbTransaction transaction = null, int? commandTimeout = null, CommandType? commandType = null)

key points here:

  • there's no point pretending IDbConnection will work; it won't; that was a bad API choice in the past
  • no buffered parameter

if you care to refactor to just the above, we should be able to take that, but I suspect it may be easier to start from a clean slate and just re-apply the interesting bits; alternatively I'm happy to take this and re-do from scratch

@mgravell
Copy link
Member

mgravell commented Jun 9, 2023

on the TFM change: I don't see why we need System.Linq.Async - if we just use Microsoft.Bcl.AsyncInterfaces, we get to keep all the TFMs IIRC

@mgravell
Copy link
Member

mgravell commented Jun 9, 2023

this feature now essentially provided via #1912; I propose we close this and re-evaluate any missing requirements when that is merged

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

Successfully merging this pull request may close these issues.

QueryAsync missing buffered parameter
2 participants