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

Video Resumable uploader #489

Closed

Conversation

@YuanhuiChen
Copy link

@YuanhuiChen YuanhuiChen commented Aug 17, 2015

Finally comes another update. Sorry I opened a new branch. You cannot see the old comments. I committed to some others' diff by mistake in the old branch.
The new changes:

  1. add integration test
  2. distinguish between no resumable exception and resumable exception in the transfer phase.
  3. fix some other issues.

Thanks again for your patience!

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Aug 17, 2015

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@gfosco
Copy link
Contributor

@gfosco gfosco commented Aug 18, 2015

This is great, and I'm willing to accept it as is. I wish it could go against version 5, but I guess that wasn't possible. Will leave it here for another day to give @SammyK & @yguedidi a chance to comment/nit if they want.

public function uploadVideo($target, $pathToFile, $accessToken, $maxTransferTries = 5)
{
$uploader = $this->getResumableUploader($accessToken, $maxTransferTries);
$endpoint = '/'.$target.'/videos';
Copy link
Contributor

@SammyK SammyK Aug 18, 2015

We shouldn't auto-suffix the endpoint with videos since it might not always be that and none of the other parts of the SDK auto-suffix endpoints. :)

Copy link
Contributor

@yguedidi yguedidi Aug 18, 2015

@SammyK uploading a video can only be done with an endpoint formated like /{node-id}/videos, so I think it's OK here, end-user will only need to give the node id.

Copy link
Contributor

@SammyK SammyK Aug 18, 2015

@yguedidi I see what you're saying. I'm also concerned that users will be reading the Graph docs and then try to enter the endpoint directly here. I for one wouldn't assume the SDK would add the /videos suffix for me. So I'm still against auto-suffixing. :)

Copy link
Contributor

@yguedidi yguedidi Aug 18, 2015

I'd prefer an easy to use signature with a good doc rather an opened signature to any endpoint with a pattern check in the definition :)

Personally, I'm with @yguedidi here. This uploader is designed for uploading videos. If we specify clear that 'target' means user/page id in the doc, user won't pass in a full endpoint path in.

@SammyK
Copy link
Contributor

@SammyK SammyK commented Aug 18, 2015

I did a quick scan a pointed out a few things. I think after @yguedidi has a look we should do some refactoring after it gets merged before we tag it as 5.1 :)

* @param int $target
* @param string $pathToFile
* @param string $accessToken
* @param int $maxTransferTries
Copy link
Contributor

@yguedidi yguedidi Aug 18, 2015

Add some parameter descriptions here

Copy link
Contributor

@yguedidi yguedidi Aug 18, 2015

this comment apply to all other public methods :)

@SammyK
Copy link
Contributor

@SammyK SammyK commented Aug 18, 2015

Also - can you add some docs? :)


while ($maxTransferTries > 0) {
try {
$res= $this->client->sendRequest($request)->getDecodedBody();
Copy link
Contributor

@yguedidi yguedidi Aug 18, 2015

space

@yguedidi
Copy link
Contributor

@yguedidi yguedidi commented Aug 19, 2015

@gfosco @SammyK I don't like merging "as is" and then refactor and/or fix things. I think when merging a PR, that PR should in a "ready to release" state. Just my 2 cents, will do how @gfosco want as usual :)

@SammyK
Copy link
Contributor

@SammyK SammyK commented Aug 19, 2015

@yguedidi Maybe we should send PR's to @YuanhuiChen's fork?

@yguedidi
Copy link
Contributor

@yguedidi yguedidi commented Aug 19, 2015

@SammyK it's a solution, but are we in a hurry? I think it's better to let @YuanhuiChen finish it, at least to learn more, as code review is an important part of the learning process :)
Of course, if @YuanhuiChen explicitly tell us he/she don't have the time to finish it, we'll help !

@SammyK
Copy link
Contributor

@SammyK SammyK commented Aug 19, 2015

@yguedidi No rush. Just thought we could submit some PR's to "talk code" instead of English as we try to figure out the best way to implement this feature since it's such a beast. :) But I'm OK with letting @YuanhuiChen do it without PR's from us as long as he has the time!

@yguedidi
Copy link
Contributor

@yguedidi yguedidi commented Aug 19, 2015

@SammyK sure I talk code better than english haha, but we also can't have some time, you know that too

@codingtmd
Copy link

@codingtmd codingtmd commented Aug 20, 2015

Thank you, guys, especially for @YuanhuiChen to work on this task! I just left some comments in diff. We are pretty close, wow!

@gfosco
Copy link
Contributor

@gfosco gfosco commented Aug 22, 2015

I'm just the shepherd :) ... I am quite happy to see all the discussion and progress, and whenever you're all ready, we'll get 5.1 out. 👍

@gfosco
Copy link
Contributor

@gfosco gfosco commented Sep 3, 2015

@YuanhuiChen will you be able to make the requested changes? Thanks

@codingtmd
Copy link

@codingtmd codingtmd commented Sep 4, 2015

@gfosco, I just heard yesterday that @YuanhuiChen already moved to another high priority task and she will not able to finish this diff. We are short of resource right now. Just wonder, is it possible to find some love from your side on this diff? Otherwise, we need to wait for another two weeks before we can find a new resource.

Cc @SammyK, @yguedidi

@SammyK
Copy link
Contributor

@SammyK SammyK commented Sep 4, 2015

I might have some time next week if we can't get anyone else on it. :)

@codingtmd
Copy link

@codingtmd codingtmd commented Sep 6, 2015

Great! Thanks, @SammyK.

@codingtmd
Copy link

@codingtmd codingtmd commented Sep 14, 2015

Hello @SammyK, how things going? :)

@SammyK
Copy link
Contributor

@SammyK SammyK commented Sep 14, 2015

Hey @codingtmd! Thanks for the ping! I didn't get a chance to look at this last week so I'm going to see what's involved with this tomorrow and let you know. :)

@SammyK
Copy link
Contributor

@SammyK SammyK commented Sep 16, 2015

Hey @YuanhuiChen! If I send a PR to your branch for this feature, will you be able to merge any changes so we can continue the discussion here? :)

@codingtmd
Copy link

@codingtmd codingtmd commented Sep 16, 2015

Hello @SammyK, @YuanhuiChen already left this project, so I'm not sure whether she will response or not. Is it possible to send a PR on the master?:)

@SammyK
Copy link
Contributor

@SammyK SammyK commented Sep 21, 2015

I think I'll try to create a new branch from @YuanhuiChen's fork so that all the existing commits will stay in place. Things got busy all of a sudden on my end but I'm still trying to work on this when I can. :)

@SammyK SammyK mentioned this pull request Sep 22, 2015
@SammyK
Copy link
Contributor

@SammyK SammyK commented Sep 22, 2015

Moved this PR to #498 so we can close this one. :)

@gfosco gfosco closed this Sep 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants