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

Apply pagination macro to all requests #78

Merged
merged 2 commits into from
May 30, 2024
Merged

Conversation

kevin-pease
Copy link
Contributor

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Fixes #59.

  • Applies the pagination macro in all endpoints (except accounts, see below), and removes duplicate code. Some tests needed to be updated slightly (for example, unwrap needed to be appended in some locations.

  • Adds a check in the pagination macro itself (set_cursor), which was previously present in some pagination implementations. This way, one uniform error message will always be returned.

  • Updates relevant documentation examples with the addition of required crate imports for the macro.

Note: Accounts endpoint still needs to be refactored, but the pagination macro needs to be updated first, due to generics. See #70.

⤵️ What is the current behavior?

Similar implementations of pagination functionality are present in some endpoints, which results in duplicate code. Also, there are minor differences between these implementation, leading to inconsistencies.

🆕 What is the new behavior (if this is a feature change)?

Common code for pagination is injected with a derive macro, rather than implemented wherever required.

💥 Does this PR introduce a breaking change?

No.

🤔 Checklist before submitting

  • All projects build
  • Follows style guide lines
  • Relevant documentation was updated
  • Rebased onto current main

Copy link
Collaborator

@tluijken tluijken left a comment

Choose a reason for hiding this comment

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

2 vragen die wellicht buiten scope van deze PR vallen:

  • Kunnen we de paginatable trait weghalen? Nu moet je een extra using toevoegen:
use stellar_rust_sdk_derive::Pagination;
use stellar_rs::Paginatable;  // deze mag wat mij betreft komen te vervallen

Kunnen we met de derive trait ook members aan de struct toevoegen?

Verder ziet het er logisch uit.

@@ -51,7 +54,7 @@ pub struct AllAssetsRequest {

/// Specifies the maximum number of records to be returned in a single response.
/// The range for this parameter is from 1 to 200. The default value is set to 10.
limit: Option<u32>,
limit: Option<u8>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Onderzoeksvraagje: kunnen we ook members aan een struct toevoegen middels de DeriveMacro?

@tluijken tluijken merged commit b31edfc into develop May 30, 2024
2 checks passed
@tluijken tluijken deleted the 59-pagination-macro branch May 30, 2024 09:29
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.

Apply pagination macro to all requests
2 participants