Allow chunked uploads to work with IO streams other than files#110
Allow chunked uploads to work with IO streams other than files#110jcmfernandes wants to merge 7 commits intocburnette:masterfrom
Conversation
add_development_dependency states that dependencies are necessary when developing an app using this gem, as in, these gems are added to the development group. We don't need that.
This was failing on my side. Was test implicitly defined in previoous RSpec versions?
The title says it all.
|
|
||
| def chunked_upload_create_session_new_file_from_io(io, parent, name) | ||
| def chunked_upload_create_session_new_file_from_io(io, parent, name, io_size: nil) | ||
| io_size ||= io.size |
There was a problem hiding this comment.
One thing to keep in mind is that #size is not part of IO's public interface.
|
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 |
|
Hey @xhocquet thanks for the quick reply! Regarding the Gemfile/gemspec stuff, I should probably write a blog post on this 😄 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 When you add 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 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 |
Doing so can actually backfire. PaaS like heroku run bundle install with `--without development:test`, effectively excluding gems in groups development and test. So having gem parallel declared as development dependency would cause `require "parallel"` locally but then fail once pushed to the PaaS.
|
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 😅 but I bumped into that post in Rails' discuss and well, I had to scratch the itch. Still on the Reiterating that it doesn't make sense to cut a new release without first merging #106. |
|
Do you need any help testing this change? |
3 participants
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 😅 - 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.