Skip to content
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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

victormarante
Copy link

@victormarante victormarante commented May 6, 2024

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:

  • It might be confusing for users to provide both the query options AND page size, as they might enter the same information twice (if they provided MaxItemCount in query options to begin with). I do not know what your thought is about this.
  • Are you ok with the method overloading (i.e. that I simply call the new method from the existing API), with same logic + a default instance of QueryOptionsClass

Tasks

  • Write tests
  • Add default values for new API to protect user from un-wanted side-effects

@victormarante victormarante marked this pull request as draft May 6, 2024 09:49
Copy link
Owner

@IEvangelist IEvangelist left a 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.

@victormarante
Copy link
Author

victormarante commented May 7, 2024

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.

Not entirely sure what you mean by this. Looking at the code, I have added 4 new methods to the IReadOnlyRepository, where in each case the existing functions simply call the new overloads with a default QueryRequestOptions (either passing nothing or the MaxItemCount, as this was previously configured).

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 QueryRequestOptions? I think this will have no impact as this already happens under the hood (to my understanding).

@IEvangelist
Copy link
Owner

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!

@IEvangelist
Copy link
Owner

@all-contributors please add @victormarante for code

Copy link
Contributor

@IEvangelist

@victormarante already contributed before to code

@victormarante
Copy link
Author

Hi, great, thanks for reviewing this far. I would like to add some tests but unsure what exactly you prefer in terms of tests?

@IEvangelist
Copy link
Owner

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)
Copy link

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or throw exception

Copy link
Owner

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?

Copy link

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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants