Excel Exporter - support for options #37

athityakumar · 2017-07-11T08:41:26Z

No description provided.

test.xls was produced while trying out in pry, and was mistakenly added into git history

zverok · 2017-07-19T15:28:53Z

(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.

zverok · 2017-07-19T15:51:30Z

lib/daru/io/exporters/excel.rb

+        #     "dataframe_df.xls",
+        #     data_options: normal,
+        #     header_options: heading
+        #   ).call


I'm totally not sure, but don't you think it is something else? Like "format of ...":

Daru::IO::Exporters::Excel.new(formatting: {header: blah, data: blah}).call(path) # or even... Daru::IO::Exporters::Excel.new(formatting: {order: blah, index: blah, data: blah}).call(path)

(BTW, as you can see by my example, I am totally not sure where path should be passed... We can discuss it later, though, leave your version)

Currently, all arguments are given only in the initialize() method and none to call().

Daru::IO::Exporters::Excel.new( df, 'path/file.xls', formatting: {header: {...}, data: {...}, index: {...}}, display: {header: true, index: false, data: true} )

Would it be better to rename :header to :order?

Currently, all arguments are given only in the initialize() method and none to call().

OK, let's leave it this way, at least for now.

Would it be better to rename :header to :order?

¯\_(ツ)_/¯ I can imagine a lot of pro and contra arguments. But probably we should stick to "dataframe point of view" (data, index, order). But² maybe "index & orders" could be called "headers" in this context.

zverok

I know it is EXTREMELY bad time to ask, but why you've choose spreadsheet gem?.. It doesn't looks reasonable for me, TBH.

zverok · 2017-07-28T14:03:45Z

lib/daru/io/exporters/excel.rb

+        #
+        # @option options formatting [Hash<Hash>] A Hash of formatting options. Supported keys
+        #   are +:header+, +:data+ and +:index+. For example, if +:header+ is set to a Hash
+        #   containing keys such as +:color+, the headers are written with the specified color.


Two hashes with same keys looks suspicious to me... Maybe we can join them, this way:

to_excel(dataframe, 'test.xlsx', header: false, # or nil, don't show index: {color: :red, bold: true}, # show, custom formatting data: true # show, default formatting, can be omitted

WDYT?..

Acknowledged, this does seem much more intuitive to use. Made this and the below changes with commit 6fe329a.

zverok · 2017-07-28T14:24:21Z

lib/daru/io/exporters/excel.rb

+        #       index:  { color: :green },
+        #       data:   { color: :blue }
+        #     },
+        #     display: { index: false }


...for example, this example is weird, first set formatting of index, then tell we don't want to show it ;)

Hmm, the use-case I was considering is when a daru-io user decides to fix their :formatting and just decide to play-around with the :display hash.

BTW, after having gone through the daru-view repository during the review discussion, I came across some frequently-used default options being declared as a class variable like Daru::View.adapter = :highcharts. Thinking about it, could it be used to define some default options? (Maybe, coupled with meta-programming?) That is, something like Daru::IO::Exporters::Excel.index = {color: :green}? Or does this seem complex?

zverok · 2017-07-28T14:24:38Z

lib/daru/io/exporters/excel.rb

-          @options   = options
+          @path       = path
+          @display    = options[:display]    || {}
+          @formatting = options[:formatting] || {}


zverok · 2017-07-28T14:25:48Z

lib/daru/io/exporters/excel.rb

-          sheet = book.create_worksheet
+          @book       = Spreadsheet::Workbook.new
+          @sheet      = @book.create_worksheet
+          @row_offset = @display[:header] ? 1 : 0


I'd say write_headers should shift the @row_offset.

zverok · 2017-07-28T14:27:44Z

lib/daru/io/exporters/excel.rb

+        def process_col_offset
+          return 0 unless @display[:index]
+          return 1 unless @dataframe.index.first.is_a?(Array)
+          @dataframe.index.first.count


Don't we have more idiomatic way to do it?.. Like

return 1 unless @dataframe.index.is_a?(MultiIndex) @dataframe.index.levels

zverok · 2017-07-28T14:29:05Z

lib/daru/io/exporters/excel.rb

-          format = Spreadsheet::Format.new color: :blue, weight: :bold
+          if @display[:index] || @display[:data]
+            @dataframe.each_row_with_index.with_index do |row_idx, i|
+              row, idx = row_idx


you can just each_row_with_index.with_index do |(row, idx), i|, probably

zverok · 2017-07-28T14:29:33Z

lib/daru/io/exporters/excel.rb

+            @dataframe.each_row_with_index.with_index do |row_idx, i|
+              row, idx = row_idx
+
+              write_index(idx, i+@row_offset) if @display[:index]


I believe those ifs place is inside write_index/write_data

zverok · 2017-07-28T14:30:06Z

lib/daru/io/exporters/excel.rb

+          @sheet.row(0).concat([' '] * @col_offset + @dataframe.vectors.to_a.map(&:to_s))
+        end
+
+        def write_index(idx, i)


Why , i if it should be row or at least r?..

zverok · 2017-07-28T14:30:30Z

lib/daru/io/exporters/excel.rb


-          book.write(@path)
+        def write_data(row, i)


zverok · 2017-07-28T14:36:28Z

lib/daru/io/exporters/excel.rb


-          book.write(@path)
+        def write_data(row, i)
+          (@col_offset..(@col_offset + @dataframe.vectors.to_a.size-1)).each do |x|


vectors.to_a.size, seriously? More idiomatic way?..

Instead of -1, you should use just excluding range (...). Or you can also do @dataframe.ncols.times. Or even row.size.times

x is extremely bad name

athityakumar · 2017-07-28T18:43:45Z

@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.

YARD Docs have been updated. Using the :data, :index, :header keyword arguments seems to be much more easier and intutive than using something like options[:display][:data]. Code quality has been improved, as per review comments.

athityakumar · 2017-07-30T11:22:24Z

@zverok - I've made the above changes.

zverok

Mostly good, don't forget about last comments.

zverok · 2017-07-30T15:44:52Z

lib/daru/io/exporters/excel.rb

+          process_offsets
+          write_headers
+
+          @dataframe.each_row_with_index.with_index do |(row, idx), i|


, r| or , dr| (delta row)

zverok · 2017-07-30T15:46:51Z

lib/daru/io/exporters/excel.rb

+        def process_offsets
+          @row_offset   = @header ? 1 : 0
+          @col_offset   = 0 unless @index
+          @col_offset ||= @dataframe.index.is_a?(Daru::MultiIndex) ? @dataframe.index.levels.size : 1


In fact, @dataframe.index.width for MultiIndex

zverok · 2017-07-30T15:50:54Z

lib/daru/io/exporters/excel.rb

+          @sheet.row(row).concat(idx.is_a?(Array) ? idx.to_a.map(&:to_s) : [idx.to_s])
+          return unless @index.is_a?(Hash)
+
+          @col_offset.times { |col| @sheet.row(row).set_format(col, Spreadsheet::Format.new(@index)) }


For a bit cleaner code, I'd define helper method:

def format(col_range, row, format) return unless format.is_a?(Hash) .... end

Then here you can just

format(0...@col_offset, row, @index)

and below

format(@col_offset...@col_offset + @dataframe.ncols, idx, @data)

WDYT?..

Yes, this looks much better & DRYier. I've added this with formatting method, and other methods like write_data, write_index redirect to formatting with different arguments. 👍

zverok · 2017-07-30T19:10:59Z

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 · 2017-07-30T19:19:34Z

@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.)

athityakumar added the in progress label Jul 11, 2017

Passes user-given options to Spreadsheet::Format

365ee8e

athityakumar force-pushed the excel-exporter-improvement branch from f46cff2 to 365ee8e Compare July 11, 2017 08:46

Removes unnecessary test.xls

d67f6d2

test.xls was produced while trying out in pry, and was mistakenly added into git history

athityakumar added the Exporter label Jul 14, 2017

athityakumar requested review from zverok, v0dro and lokeshh July 14, 2017 07:44

zverok reviewed Jul 19, 2017

View reviewed changes

athityakumar added 2 commits July 20, 2017 03:12

Merge branch 'master' into excel-exporter-improvement

36a9b80

Adds support to export DataFrame's Index / MultiIndex to .xls file

12d5d95

zverok requested changes Jul 28, 2017

View reviewed changes

zverok approved these changes Jul 30, 2017

View reviewed changes

Adds generic formatting method

e3f331a

athityakumar mentioned this pull request Jul 30, 2017

Excel Exporter - gem dependency #43

Open

athityakumar merged commit 67f0d6b into master Jul 30, 2017

athityakumar mentioned this pull request Aug 4, 2017

Modules to_excel and from_xlsx #9

Closed

athityakumar removed the in progress label Aug 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Excel Exporter - support for options #37

Excel Exporter - support for options #37

athityakumar commented Jul 11, 2017

zverok commented Jul 19, 2017

zverok Jul 19, 2017

athityakumar Jul 20, 2017

zverok Jul 20, 2017

zverok left a comment

zverok Jul 28, 2017

athityakumar Jul 28, 2017

zverok Jul 28, 2017

athityakumar Jul 28, 2017

zverok Jul 28, 2017

zverok Jul 28, 2017

zverok Jul 28, 2017

zverok Jul 28, 2017

zverok Jul 28, 2017

zverok Jul 28, 2017

zverok Jul 28, 2017

zverok Jul 28, 2017

athityakumar commented Jul 28, 2017

athityakumar commented Jul 30, 2017

zverok left a comment

zverok Jul 30, 2017

zverok Jul 30, 2017

zverok Jul 30, 2017

athityakumar Jul 30, 2017

zverok commented Jul 30, 2017

athityakumar commented Jul 30, 2017

Excel Exporter - support for options #37

Excel Exporter - support for options #37

Conversation

athityakumar commented Jul 11, 2017

zverok commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zverok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athityakumar commented Jul 28, 2017

athityakumar commented Jul 30, 2017

zverok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zverok commented Jul 30, 2017

athityakumar commented Jul 30, 2017