-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
Feature/support continuation token limit property #437
base: main
Are you sure you want to change the base?
Feature/support continuation token limit property #437
Conversation
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.
Looks okay thus far... Looks like some of these are new APIs and one is modified (PageAsync
). This is a breaking change, it's probably better to add all new APIs.
src/Microsoft.Azure.CosmosRepository/Repositories/DefaultRepository.Paging.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Azure.CosmosRepository/Repositories/DefaultRepository.Paging.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Azure.CosmosRepository/Repositories/DefaultRepository.Paging.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Azure.CosmosRepository/Repositories/DefaultRepository.Paging.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Azure.CosmosRepository/Repositories/DefaultRepository.Specs.cs
Outdated
Show resolved
Hide resolved
Not entirely sure what you mean by this. Looking at the code, I have added 4 new methods to the EDIT: Or are you referring to the fact that I have moved the logic to the new APIs, and let the existing APIs call the new overloads with a default |
Ok, sounds good. When I was first looking at this, it wasn't clicking that those were the changes. That makes sense, and that's what I was hoping for. This is good! |
@all-contributors please add @victormarante for code |
@victormarante already contributed before to code |
Hi, great, thanks for reviewing this far. I would like to add some tests but unsure what exactly you prefer in terms of tests? |
There are a lot of tests that already exist, rely on those for inspiration for new tests. |
.ConfigureAwait(false); | ||
|
||
// make sure that if the user hasn't said the value already we take it from the pageSize parameter | ||
if (requestOptions.MaxItemCount is null) |
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.
You must expect that null could be passed. Initialize requestOptions if the value is null.
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.
Or throw exception
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.
But the requestOptions
isn't nullable, are you implying that we should protect against it anyway?
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.
By default, I don't trust users 😂 But if for you it's ok then for me too 😄
This PR will add support for the Cosmos SDK API QueryOptionsClass which enables consumers of these APIs to provide their own settings for the
CosmosClient
.This PR aims to solve: #432
@IEvangelist I have made an initial PR here (no tests or so yet) in order to make sure I am on the right track before spending more time on this. I have two questions that will need discussion:
QueryOptionsClass
Tasks