-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add cql batch inserts #110
Conversation
Total Coverage: 45.06% Coverage Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments.
Overall, I think it's not a good idea to provide rate limit as string since it can be difficult to parse correctly and it can be error-prone for the developer in code writing process. I would suggest constructing AsyncRateLimitPolicy
directly. We can always make a conversion from string policy to AsyncRateLimitPolicy
later.
ExecuteBatch(batch, i, rateLimit, cancellationToken); | ||
} | ||
|
||
return Task.FromResult(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about error handling? :)
test/Storage/CqlTests.cs
Outdated
} | ||
|
||
[Theory] | ||
[InlineData("1000 per second", true)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to make a dedicated test for rate limit parsing (with negative cases).
Also, defining a policy that way is not idiomatic and can cause interoperability issues with other dotnet libraries. Like, if I got a TimeSpan from somewhere else, how can I pass it to this method? |
Total Coverage: 44.74% Coverage Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better now :)
No description provided.