Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Сlient module #140
Сlient module #140
Changes from all commits
7d1c22b
c6562af
7630412
31a0bf0
c732bae
be07703
739912d
7a275a2
ed8fda8
46d9c57
0469396
8821a2f
f060644
c44cffc
1d8a5f8
0647db7
2733d69
7377a3f
990ad20
d587f46
fdee543
7f6abce
1d83880
baa5c6c
30123e3
6d2ab01
c708009
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is correct for Franklin's implementation of pagination, but only for Franklin's -- the particular pagination strategy is left open to implementers. So there are a few things that are a bit weird as a result --
next
will be a jsonPaginationToken
, since it will just be an opaque string by the time it reaches the client. This means this is probably wrong for any other STAC API implementation.https://github.com/azavea/franklin/blob/f5be8ddf48661c5bc43cbd22cb7277e961641803/application/src/main/scala/com/azavea/franklin/api/schemas/package.scala#L84-L85
So I think this shouldn't work
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'd say this is fine; since it is a default implementation (i.e. if smth doesn't work user can always define theirs own STACClient with a redefined search filters: see https://github.com/azavea/stac4s/blob/7f6abceeebea59dfc9a605891c73fc121116610b/modules/client/jvm/src/main/scala/com/azavea/stac4s/api/client/SttpStacClient.scala#L9-L13 - they only need to define their own version of
SearchFilters
), if smth won't work in the future with other STAC Services I think we should handle it later (?).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.
Opened an issue for that #198
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'm not really talking about streaming here though -- I don't think this pagination encoding will work, and the tests included here don't exercise pagination. Since the pagination implementation here isn't exercised and I think won't work, I think it should just be omitted until someone is working on #198
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.
Oops forgot to push not tested codecs
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.
@jisantuc added it into the issue description #198