Add: options parameter to zipline

benkoshy commented 3 years ago

What is this commit?

Allows for options to be passed in (if required).

Why do we all need it?

Sometimes we may need to pass in options in order to access specific functionality.

In my particular case, I want to pass in the options to allow for duplicate file names to be renamed. The default is false but I would like it to be true. Things will still work with the default, as it was before.

zipline(@documents.map{ |d| [d.file, d.filename]}, "name.zip", auto_rename_duplicate_filenames: true)

Those options are then passed on to the delegated Ziptricks streamer and everyone's happy!

Tests

I did not find a test for the zipline helper, so I have simply passed in the parameter and tested it manually on my end. I'm not sure how we would test the Zipline::zipline helper method.

Please let me know if there are any issues and I hope to be able to work towards clearing them up.

Ben

aried3r commented 3 years ago

Hey @benkoshy, since you are working on the options and using ** to pass them along. Did you by any chance also get deprecation warnings with Ruby 2.7 regarding this call?

https://github.com/fringd/zipline/blob/264c6ae6b3818495b81a3340540d1e49af4893fe/lib/zipline/zip_generator.rb#L83

.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/zipline-1.3.0/lib/zipline/zip_generator.rb:83: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/zip_tricks-5.5.0/lib/zip_tricks/streamer.rb:339: warning: The called method `write_deflated_file' is defined here

PS: Oh, I didn't know about deduplication in zip_tricks, we've implemented our own deduplication code in the meantime, but this PR would be a great way to remove our own code.

benkoshy commented 3 years ago

@aried3r

Hey @benkoshy, since you are working on the options and using ** to pass them along. Did you by any chance also get deprecation warnings with Ruby 2.7 regarding this call?

Curiously, when running the tests, I didn't receive those deprecations warnings. It looks like the options referred to are not tested directly, so no deprecation warnings are triggered when running the tests.
NB the options passed to the streamer.write_deflated_file(name, options) are different to the Streamer initialization options (see here: https://github.com/fringd/zipline/blob/ac1977eec593aa59e3cafbd05fae29f20f17b14f/lib/zipline/zip_generator.rb#L33), so no deprecation warnings would be triggered.
but if you call those method directly, yeah make sense that deprecation warnings would be triggered, which could be worth a separate PR.

Having said that, it looks like there are no tests associated with the helper method. I might add one to ensure that the options are in fact passed, and initialized correct.

courtsimas commented 2 years ago

👀

benkoshy commented 2 years ago

@fringd Hey Ram, is there anything you need me to do, in order to merge this? I had actually forgotten about it, but will be revisiting this issue in a few weeks. Pls LMK.

summera commented 2 years ago

Would be nice to get this merged sometime soon! @fringd anything we can do to speed up the process? Thanks!

fringd commented 2 years ago

hey sorry for the slow response. I've resolved the conflicts but see some failing tests. Could someone take a look and resolve those?

fringd commented 2 years ago

one more request. could you include a test that exercises this functionality? just verify that argments are passed along to zip tricks?

fringd commented 2 years ago

sorry i got the tests more or less good for you. they were bad on master for some reason. i think something's wrong with ruby head, disabled it for now

fringd / zipline