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

refactor(store): HistoryQuery.direction #2263

Merged
merged 6 commits into from
Dec 19, 2023

Conversation

AlejandroCabeza
Copy link
Contributor

Description

Update HistoryQuery.ascending (and related) references to direction so they are consistent with RFC naming. Also, fix a default value issue with it.

Changes

  • Update ascending references to direction
  • Fix default value issue with HistoryQuery.direction

Copy link

github-actions bot commented Nov 30, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2263

Built from f8677e4

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!
It looks great! Besides, I think it would be a bit better if we could use the following type when we refer to a "pagination direction", if that's feasible of course

PagingDirectionRPC* {.pure.} = enum

tests/waku_archive/test_waku_archive.nim Outdated Show resolved Hide resolved
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks! Just added a minor comment

waku/waku_api/jsonrpc/store/handlers.nim Outdated Show resolved Hide resolved
@AlejandroCabeza AlejandroCabeza requested review from NagyZoltanPeter and removed request for alrevuelta December 13, 2023 14:53
@AlejandroCabeza AlejandroCabeza marked this pull request as draft December 13, 2023 15:18
@AlejandroCabeza AlejandroCabeza marked this pull request as ready for review December 13, 2023 16:16
Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

It is great, thank you. I realized how rest and rpc namings are differ but its not an issue to change. We will remove sometime rpc thus we should not break any interface just because of it.

@AlejandroCabeza AlejandroCabeza merged commit fae20bf into master Dec 19, 2023
9 of 10 checks passed
@AlejandroCabeza AlejandroCabeza deleted the history-query-ascending branch December 19, 2023 14:10
Ivansete-status pushed a commit that referenced this pull request Jan 9, 2024
* Fix issue with default history query ascending value in serde operations: Should use the same value.
* Update direction types to PagingDirection.
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