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.

Sign up for GitHub

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

Jump to bottom

fix(download): Fix sort for download endpoint, and add doc #38

Merged
merged 3 commits into from
Jun 8, 2019
Merged

fix(download): Fix sort for download endpoint, and add doc #38

merged 3 commits into from
Jun 8, 2019

Conversation

qingyashu
Copy link
Contributor

add more doc

New Features

Breaking Changes

Bug Fixes

Improvements

  • add more doc about download endpoint

Dependency updates

Deployment changes

ZakirG
ZakirG previously approved these changes Jun 7, 2019
View reviewed changes
Copy link
Contributor

@ZakirG ZakirG left a comment

Choose a reason for hiding this comment

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

Thanks :)

mfshao
mfshao reviewed Jun 7, 2019
View reviewed changes
Copy link
Contributor

@mfshao mfshao left a comment

Choose a reason for hiding this comment

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

doesn't Guppy's endpoint also need to supply "index" in the payload?

might be good to include an example request, like something you have in the guppy group text google doc?

@qingyashu
Copy link
Contributor Author

Thanks! We have already removed index from this endpoint since we assume each index only has one doctype, so it's sufficient to only have type arg. @mfshao

I have also found a bug for sort arg in download endpoint, and fixed in this PR. Please also take a review (would appreciate if test it, too) . 🙏

@qingyashu qingyashu requested a review from m0nhawk June 7, 2019 21:46
@qingyashu qingyashu changed the title fix(doc): add download endpoint doc fix(download): Fix sort for download endpoint, and add doc Jun 7, 2019
mfshao
mfshao approved these changes Jun 7, 2019
View reviewed changes
Copy link
Contributor

@mfshao mfshao left a comment

Choose a reason for hiding this comment

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

👍 awesome!

did some manual tests locally and looks like it works. I'm approving it

m0nhawk
m0nhawk reviewed Jun 7, 2019
View reviewed changes
src/server/es/index.js
@@ -82,6 +82,11 @@ class ES {
let totalData = [];
let batchSize = 0;

// This is really ridiculous that ES's JS library has it, but we need to
Copy link
Contributor

Choose a reason for hiding this comment

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

JS can fun sometimes. 😃

@qingyashu qingyashu merged commit 774b48c into master Jun 8, 2019
@qingyashu qingyashu deleted the fix/doc-download branch June 11, 2019 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m0nhawk m0nhawk m0nhawk approved these changes

@mfshao mfshao mfshao approved these changes

@ZakirG ZakirG ZakirG left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qingyashu @mfshao @ZakirG @m0nhawk