Added Query iterator method Continuation #89
Conversation
Updated System.Text.Json package because of compile errors (warning as errors)
|
There was a problem hiding this comment.
@BlackGad thanks, see comments below plus the unresolved comments on the original PR (#80).
But importantly, as I wrote there, please add tests - that's a vital part of implementing any new feature. Otherwise we can't be sure this actually works, and it may also accidentally break in the future without us knowing about it.
| @@ -554,7 +564,7 @@ public Task<FlushResult> FlushAsync(CancellationToken cancellationToken = defaul | |||
| Value = Math.Min(batchSize, leftItemsCount).ToString(CultureInfo.InvariantCulture) | |||
| }); | |||
|
|
|||
| var nextExpression = pkField.DataType == DataType.VarChar | |||
| string nextExpression = pkField.DataType == DataType.VarChar | |||
There was a problem hiding this comment.
Please do the same switch here as above, with an exception for unsupported types etc.
| @@ -554,7 +564,7 @@ public Task<FlushResult> FlushAsync(CancellationToken cancellationToken = defaul | |||
| Value = Math.Min(batchSize, leftItemsCount).ToString(CultureInfo.InvariantCulture) | |||
| }); | |||
|
|
|||
| var nextExpression = pkField.DataType == DataType.VarChar | |||
| string nextExpression = pkField.DataType == DataType.VarChar | |||
There was a problem hiding this comment.
In the concatenation below (nextExpression += $" and {userExpression}"), aren't you missing parentheses around the user expression, to prevent any possible operator precedence issues?
|
Tests added |
|
Seems handled all the comments from previous PR. |
|
What is strange that my local unit tests run is successful
|
|
It seems like there's a race condition error because I added the first tests for string keys. Do you want me to remove the tests for string key schemas? EDIT: My bad it was copy paste issue in tests. Forget to change collection name |
|
Thanks - but note that there are still test failures. I'll try to review again soon, though note I'll be away for the coming week, so it will likely take a bit of time. You'll have to be patient. |
Test run now successful. Am I missed some comments? Or we finished so far? |
|
Thanks, I'm going to merge this. I'll likely do another small pass next week with some cleanup/stuff before releasing. |
|
@roji Thank you! |
|
@roji Hi! When release is planned? Still old version at NuGet.org |
|
Also looking forward to the next release - the progress has been plodding lately, are there any plans to continue the development of the library? |
Initial PR #80
Here is comments fix and System.Text.Json package update because of compile errors (warning as errors )