boxr
boxr copied to clipboard
cburnette
Allow chunked uploads to work with IO streams other than files
I bumped into https://discuss.rubyonrails.org/t/transferring-files-from-s3-to-box-in-chunks-without-a-tmp-file/78841 as it landed on my inbox - major coincidence :sweat_smile: - and just like reported in #106, chunked uploads aren't currently working for IO streams other than files. This PR fixes that, and I took the chance to do some small refactoring, extracting some methods out of #chunked_upload_to_session_from_io.
Hey @jcmfernandes , thanks for the contribution! I'll definitely be taking a look since this seems like a great quality of life improvement.
At first glance, the things that concern me the most are the Gemfile changes and added dependency. When developing gems, smart people tell us to stick to mostly-empty Gemfiles and instead stick to the gemspec to keep everything in one place.
As for the stringio addition, I don't know enough about what it's doing to know whether it's absolutely necessary or not. The goal of course is to have as few dependencies as possible and to keep this gem nice and small!
Hey @xhocquet thanks for the quick reply!
Regarding the Gemfile/gemspec stuff, I should probably write a blog post on this :smile: it's my understanding that the blog post you referenced is somewhat outdated. With that said, one thing is very much up to date: do not check-in Gemfile.lock when developing a gem.
When you add spec.add_development_dependency "xpto" to a gemspec, that's the equivalent of adding gem xpto to group :development in the Gemfile of the application/other gem that is including your gem as a dependency. This sentence is a bit confusing but I can't find another way of making it clearer. So, add_development_dependency has nothing to do with the development of your gem, but with the development of other apps/gems that include your gem. A good example is codecov. We use codecov here to assess this code's coverage. It's a development tool in the context of this gem. We don't need to "force" others depending on our gem to also have codecov as one of their dependencies, not even as a development dependency.
Furthermore, sometimes it's relevant to have the same gem referenced in the gemspec and Gemfile. That usually happens when you want to change the source of the gem; say you want to install your fork of codecov that lives in your github account. In that case, you would add spec.add_development_dependency "codecov" to the gemspec and gem "codecov", git: "https://github.com/..." to your Gemfile below the call to gemspec.
Long, possibly confusing, sorry about that.
Regarding stringio, it's part of ruby's standard library. It's necessary as we need to create IO streams that are backed by strings, and that's exactly what stringio is for.
@jcmfernandes Thanks for the context, definitely some stuff to chew on and it's definitely possible that the top google result is outdated! It was my understanding that using gemspec would not force apps using this gem to require the development gems when installing, so I'll need to go reread the docs to clarify my understanding. The goal of course is that clients don't download any dependencies besides the 4 minumum today (5 if you want parallel chunked uploads)
Regarding stringio, I think I just mixed it up with the dependencies, my mistake!
No worries! Being 100% honest, I actually don't use this gem as of today. I had to create a new box developer account to run the test suite :sweat_smile: but I bumped into that post in Rails' discuss and well, I had to scratch the itch.
Still on the #add_development_dependency topic, in https://github.com/cburnette/boxr/pull/110/commits/98903aa06ea11ee133226b302bf3b46fabc67b91 I detail why I'm doing the change. I hope it makes sense.
Reiterating that it doesn't make sense to cut a new release without first merging #106.
