search

SciRuby / daru-io

daru-io is a plugin gem to the existing daru gem, which aims to add support to Importing DataFrames from / Exporting DataFrames to multiple formats.
http://www.rubydoc.info/github/athityakumar/daru-io/master/
MIT License
24 stars 9 forks source link

Excel Exporter - support for options #37

Closed athityakumar closed 7 years ago

zverok commented 7 years ago

(Before the review) Please-please, try to do the descriptive descriptions (sorry for tautology) for your PRs. To write "This enchances this and that, while leaving that and this intact" will take just 5 minutes of your time, but all reviewers would be really grateful.

athityakumar commented 7 years ago

@zverok - The existing Excel Exporter was already based on 'Spreadsheet' gem, and I didn't want to break any backward dependencies by changing it. Also, I didn't find any compelling reason to change the Excel Exporter to depend on a different gem. Hence, I chose to stick with the 'Spreadsheet' gem itself. If I'm missing out on any issue with using 'Spreadsheet' gem / if there are better alternatives that you'd like to suggest, please feel free to suggest. 😄

Regarding rest of the review comments, acknowledged. I'll soon make the changes.

athityakumar commented 7 years ago

@zverok - I've made the above changes.

zverok commented 7 years ago

OK, let's merge it.

But please add issue about underlying gem. The problem is, there are basically 2 (absolutely different) formats of Excel files: old binary one (.xls) and new XML-based one, standartized and modern (.xlsx). If I am not missing something, spreadsheet gem works with the former (which also known as "Excel-2003 format", e.g. 14 years outdated, though still in use), for working with latter you'll need axlsx or rubyxl.

athityakumar commented 7 years ago

@zverok - Acknowledged. I've opened #43 for future discussions on the gem dependency. But, can't XLSX Exporter simply have a different dependency like how the XLSX Importer (PR #28) depends on Roo? (Ofcourse, lesser dependencies would be better.)