search

Jesus / dropbox_api

Ruby client library for Dropbox API v2
MIT License
171 stars 113 forks source link

Feature/get thumbnail batch #40

Closed kilgore5 closed 6 years ago

kilgore5 commented 6 years ago
Jesus commented 6 years ago

Hi,

thanks for your PR, I'll go through it during this week and try to get it merged.

kilgore5 commented 6 years ago

Hi @Jesus Thanks for your quick reply and your work on this gem!

I wanted to add some thoughts/questions:

Jesus commented 6 years ago

I aimed to work with the conventions and patterns as much as possible. I'm all ears on feedback for semantics or anything else

I think you did a very good job, did you find it difficult to understand the code?

I waffled a bit on how to handle the GetThumbnailBatchResult and whether it should behave more like the GetAccountBatch and just return the entries array directly. Any feedback appreciated.

I like the way you did it. I guess you're referring to the fact that some endpoints put the results as a plain list (like get_account_batch) and others use the entries property (like list_folder).

I understand it's confusing. My rule is to mirror exactly what's provided by the plain Dropbox API, which you did in this case so all good :)

I wasn't 100% sure how to handle the errors for the individual thumbnails since they can be interspersed with the data entries and they're not "raised." Any feedback appreciated.

I think it's ok in this case because the error is part of a successful response. If you raised it we may be losing other successful entries.

I wanted to add to the spec scaffold generator, but wasn't sure how you wanted to handle storing/generating a sample image

You could add a couple of small images and place them in spec/fixtures. I've just checked and the spec folder won't be included once the gem is packaged so this should be ok. Anyway please don't use big images.

Do you want to add that scaffold? Otherwise I'm happy to merge this as it is now.

Many thanks for this thoughtful contribution.

kilgore5 commented 6 years ago

@Jesus Understanding the code was fine... it's very well organized. You got the gist of my other questions, so I am good to go.

I've added the scaffold. Let me know if I need to adjust or you need anything else to merge. Thanks for your review!

Jesus commented 6 years ago

Thanks for your awesome contribution.