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

Upload album example & album user tags #1015

Merged
2 commits merged into from Mar 6, 2017

Conversation

Projects
None yet
2 participants
@Overtorment
Contributor

Overtorment commented Mar 3, 2017

No description provided.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 3, 2017

@Overtorment Thank you for contributing.

The code is very clunky and buggy, though... So it won't be merged the way it is. Remember that this is a library to be used by everyone. It shouldn't have complex methods of assigning info.

Right now you're forcing people to write 0 => [] tags for first photo, 1 => [] tags for second photo etc. Which I guess is fine as long as those array indexes perfectly match the photos array... But your code is bugged. It only increments $c++ when it finds tags for a photo offset. So You would not be able to say 0 => [] tags for first photo], 5 => [tags for fifth photo], because it would never get to the number 5. In fact if you don't start with offset 0 => you wouldn't even have tags for the first photo.

Anyway, perhaps we should merge tags with the $photos, so that $photos is:

[
    ['file'=>somefile..., 'usertags'=>[tags for that photo..]]
]

And with that format we can support further per-photo metadata much more easily.

So no, I vote for not merging the current code. Too complicated to use.

ghost commented Mar 3, 2017

@Overtorment Thank you for contributing.

The code is very clunky and buggy, though... So it won't be merged the way it is. Remember that this is a library to be used by everyone. It shouldn't have complex methods of assigning info.

Right now you're forcing people to write 0 => [] tags for first photo, 1 => [] tags for second photo etc. Which I guess is fine as long as those array indexes perfectly match the photos array... But your code is bugged. It only increments $c++ when it finds tags for a photo offset. So You would not be able to say 0 => [] tags for first photo], 5 => [tags for fifth photo], because it would never get to the number 5. In fact if you don't start with offset 0 => you wouldn't even have tags for the first photo.

Anyway, perhaps we should merge tags with the $photos, so that $photos is:

[
    ['file'=>somefile..., 'usertags'=>[tags for that photo..]]
]

And with that format we can support further per-photo metadata much more easily.

So no, I vote for not merging the current code. Too complicated to use.

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Mar 3, 2017

Contributor
Contributor

Overtorment commented Mar 3, 2017

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 3, 2017

@Overtorment Excellent! :) Well, we're working on 2.x which is allowed to make bigger changes to functions, so I suggest you go for the idea I mentioned where $photos is an array where each photo has keys for (mandatory) file and (optional) usertags.

The existence of user tags can be checked via isset($photo['usertags']). Which is true if the value exists and is not null.

ghost commented Mar 3, 2017

@Overtorment Excellent! :) Well, we're working on 2.x which is allowed to make bigger changes to functions, so I suggest you go for the idea I mentioned where $photos is an array where each photo has keys for (mandatory) file and (optional) usertags.

The existence of user tags can be checked via isset($photo['usertags']). Which is true if the value exists and is not null.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 3, 2017

Also, the usertags definition has an ['in'=>[...]] format right now. Is there any reason to expose 'in' to users instead of generating that for them? It adds clutter so if user tags always use "in" as their parent tag, then we should add it automatically for the user. ;) But I may be wrong, I have never used the tags APIs. Perhaps there are more types than just "in".

ghost commented Mar 3, 2017

Also, the usertags definition has an ['in'=>[...]] format right now. Is there any reason to expose 'in' to users instead of generating that for them? It adds clutter so if user tags always use "in" as their parent tag, then we should add it automatically for the user. ;) But I may be wrong, I have never used the tags APIs. Perhaps there are more types than just "in".

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Mar 3, 2017

Contributor
Contributor

Overtorment commented Mar 3, 2017

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 4, 2017

Ah I see. But tag removal would not be used during the initial uploadAlbum. But just for flexibility I guess it is okay to force the user to specify 'in'. That's good in case Instagram adds other types of usertags in the future. Then we would be futureproof since users could just specify whatever usertags parameters they want.

So yeah, keep the 'in' method the way you did it. But just move the per-photo usertags configurations into the $photos array as suggested, so that users get less confused and don't have to count array indexes manually to set tags per photo hehe.

Thanks and good luck. :)

ghost commented Mar 4, 2017

Ah I see. But tag removal would not be used during the initial uploadAlbum. But just for flexibility I guess it is okay to force the user to specify 'in'. That's good in case Instagram adds other types of usertags in the future. Then we would be futureproof since users could just specify whatever usertags parameters they want.

So yeah, keep the 'in' method the way you did it. But just move the per-photo usertags configurations into the $photos array as suggested, so that users get less confused and don't have to count array indexes manually to set tags per photo hehe.

Thanks and good luck. :)

@newfolders

This comment has been minimized.

Show comment
Hide comment
@newfolders

newfolders Mar 4, 2017

@Overtorment Is there any way of making album likes too?

newfolders commented Mar 4, 2017

@Overtorment Is there any way of making album likes too?

@Overtorment

This comment has been minimized.

Show comment
Hide comment
@Overtorment

Overtorment Mar 4, 2017

Contributor

@SteveJobzniak check now, feedback welcome. Decided to drop explicit in for now, end user doesnt need to know the insides of configure at this point. Otherwise remade it as you suggested.
Rebased against master. StyleCI fails are not relevant.

@newfolders I havent tested but Im guessing that regular like() should work with Albums as well, as long as you know album's id.

Contributor

Overtorment commented Mar 4, 2017

@SteveJobzniak check now, feedback welcome. Decided to drop explicit in for now, end user doesnt need to know the insides of configure at this point. Otherwise remade it as you suggested.
Rebased against master. StyleCI fails are not relevant.

@newfolders I havent tested but Im guessing that regular like() should work with Albums as well, as long as you know album's id.

@mgp25 mgp25 self-requested a review Mar 5, 2017

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 5, 2017

@Overtorment Thank you so much! Nicely structured and easy for users to understand! And we'll be able to add other album photo config fields in the future, very easily, thanks to this array method. People don't have to count photo array offsets anymore, haha. ;)

The code itself looks to be working and is well structured! I've given some coding style feedback, most importantly for the example, to help users. After those tweaks we'll happily merge! 👍

Thank you so much again for your great contribution!

ghost commented Mar 5, 2017

@Overtorment Thank you so much! Nicely structured and easy for users to understand! And we'll be able to add other album photo config fields in the future, very easily, thanks to this array method. People don't have to count photo array offsets anymore, haha. ;)

The code itself looks to be working and is well structured! I've given some coding style feedback, most importantly for the example, to help users. After those tweaks we'll happily merge! 👍

Thank you so much again for your great contribution!

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Mar 6, 2017

@Overtorment Hey, thanks again, I've got some time now. So I'll merge it and do the code cleanup myself so that we can close this pull request. Thank you very much for your work on this feature! :)

ghost commented Mar 6, 2017

@Overtorment Hey, thanks again, I've got some time now. So I'll merge it and do the code cleanup myself so that we can close this pull request. Thank you very much for your work on this feature! :)

@ghost ghost merged commit 128bf39 into mgp25:master Mar 6, 2017

1 check failed

continuous-integration/styleci/pr The StyleCI analysis has failed - 1 file needs addressing
Details
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost commented Mar 6, 2017

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment