search

DapperLib / Dapper

Dapper - a simple object mapper for .Net
https://www.learndapper.com/
Other
17.49k stars 3.67k forks source link

Added CommandDefinition overload #2052

Open terryaney opened 6 months ago

terryaney commented 6 months ago

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

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

mgravell commented 6 months ago

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?

terryaney commented 6 months ago

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?

mgravell commented 6 months ago

The change: I'm fine with

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

terryaney commented 6 months ago

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 commented 6 months ago

@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) :)

fab60 commented 4 months ago

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