Dapper icon indicating copy to clipboard operation
Dapper copied to clipboard

Added CommandDefinition overload

Open terryaney opened this issue 1 year ago • 7 comments

Added CommandDefinition overload for Task<IEnumerable<TReturn>> QueryAsync<TReturn>.

In separate library, I wanted to support CancelationToken so needed to expose this functionality.

terryaney avatar Mar 10 '24 18:03 terryaney

It might be worth checking whether QueryUnbufferedAsync is more appropriate for your scenario; this already exists and supports cancellation via WithCancellation (the standard API for that interface). Any use?

mgravell avatar Mar 10 '24 18:03 mgravell

Well, to be honest, InterpolatedSql.Dapper extensions is just a pass through to Dapper after constructing sql and parameters. He was passing through to the overload and I just tried to added CancellationToken support for it. I'm not actually using the method nor have I researched its purpose but adding the overload in Dapper seemed pretty straight forward (I'm not sure what the error message in Appveyor means as I didn't touch any test projects).

Think it is better to just ignore CancellationToken support on the overload I tried to add it to?

terryaney avatar Mar 10 '24 19:03 terryaney

The change: I'm fine with

Test failure: probably just random CI oddness, I'll look

  • needs at least one test showing it being used
  • it should have prompted you to add the public API definitions to the shipped/unshipped files; did it not?

mgravell avatar Mar 10 '24 20:03 mgravell

No, I didn't get any prompt, but not sure I did 'what I was supposed to'. Opening the repository in VS Code, I had 1000s of compile errors and didn't even have all the installed frameworks required for Dapper (I didn't investigate all the other Dapper.* projects). And I was hoping for CI on commit, which it did, so I simply changed the single CS file to add the overload following convention of other CommandDefinition overloads.

So, assuming the prompt was during the build, no, I didn't get. Test failing is probably because there isn't a test defined hitting my overload? I can go look to see if I can add a test case if that is the case.

Thanks for checking in on this.

UPDATE: I did add my overload to the tests. Will see what AppVeyor says now.

terryaney avatar Mar 11 '24 13:03 terryaney

@mgravell Also noticed that all the GridReader.Read*Async methods do not have cancellation support. Is it as easy as adding optional parameter and passing through to internal async implementation? If so I could do the mundane task of adding to all the methods if you want. If more complicated (due to backward compat), I'll leave to you (someday) :)

terryaney avatar Mar 23 '24 15:03 terryaney

@mgravell: You can close this https://github.com/DapperLib/Dapper/pull/1784 if this got merged

fab60 avatar May 09 '24 12:05 fab60