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

Adding feature: Partial range download #29

Merged
merged 7 commits into from
Sep 23, 2020

Conversation

namdien177
Copy link
Collaborator

@namdien177 namdien177 commented Aug 22, 2020

Reason and basic implementation are described in #27.

Since I am not familiar with the request package (tested with node-fetch OK ) so there might be some mistake.

Also, because there is no guide to perform testing and stuff and I have tried to test it, it seems like the uploadSimple API has some issue on the test case with parentPath.
Can you check that?

testing with E2E seems to be fine.

image

@namdien177
Copy link
Collaborator Author

namdien177 commented Aug 23, 2020

image

never mind, I figured it out.
TheReadableStream of the mockup data is already passed and ended on the previous test, and I need to restart the stream for the next test.

E2E test passed.

@namdien177
Copy link
Collaborator Author

namdien177 commented Aug 24, 2020

I found some mistake in the README.md, gonna fix it later.

should ok by now.

Copy link
Owner

@dkatavic dkatavic left a comment

Choose a reason for hiding this comment

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

This is an amazing contribution @namdien177 , Thank you 💯

Thanks for fixing the documentation. Good job on figuring out how to run e2e tests, unfortunately, OneDrive doesn't offer sandbox env that I could use for running automated e2e tests so it is not easy to figure out how to run them. They are passing for me. Let me know if you have any suggestions on how to improve the project

Great job again

@dkatavic
Copy link
Owner

@namdien177 I have added you as a collaborator on a project, when you resolve the conflict you can squash and merge 👍

@namdien177
Copy link
Collaborator Author

thanks.
By the way, I was wondering what was this PR for: Feature/add query param.
File concern: listChildren.js

As far as I understand, the query here doesn't do anything and the PR was just for adding optional params and change the README content. Am I wrong here?

If I was right then I would recommend a new PR to add the query string option to all APIs (and exclude the README changes or add a WIP tag to the option from this PR to prevent confusion).

@namdien177 namdien177 requested a review from dkatavic September 21, 2020 07:54
@dkatavic
Copy link
Owner

You are correct, query was correctly added as a parameter in this commit , but because of a bad merge it was removed here

I will submit a fix for this. I'm up for adding OData query to all APIs

@namdien177
Copy link
Collaborator Author

namdien177 commented Sep 22, 2020

You are correct, query was correctly added as a parameter in this commit , but because of a bad merge it was removed here

I will submit a fix for this. I'm up for adding OData query to all APIs

Glad to hear this feature will be coming soon.
And by the way, can you re-approve the changes? The CI build status is waiting for the status to be reported and I don't know how to change/update/fix this to finish CI so I'm assuming getting a new approval will resolve this. Can you check that for me? (or it is related to this issue?)

@dkatavic
Copy link
Owner

@namdien177 Fixed

@namdien177 namdien177 merged commit a227e52 into dkatavic:master Sep 23, 2020
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.

2 participants