Adds support for Excelx (xlsx) Exporter #64

athityakumar · 2017-10-25T21:00:55Z

Fixes a silly yet major bug with linkage between daru & daru-io calls (for ALL write exporter methods, which yet again draws attention to handle Add tests for linkages between daru & daru-io calls #55 ASAP)
Adds support to Excelx Exporter (formatting support not added yet) with documentation and tests. Current arguments include sheet and boolean values for index, header and data (show them or not). And of course, filename for write method. Fix Support for Excelx Exporter #63.
All the write methods return true incase of no mishaps when the particular exporter is called.

athityakumar · 2017-10-25T21:09:07Z

lib/daru/io/exporters/excelx.rb

+      # `Daru::DataFrame` instance variables
+      class Excelx < Base
+        Daru::DataFrame.register_io_module :to_excelx_string, self
+        Daru::DataFrame.register_io_module :write_excelx, self


Apart from standalone xlsx exporter, doesn't it make sense for the excel exporter to redirect here incase of .xlsx filename in excel exporter's write method? Though, for such a redirect to be possible, they would need to have similar kind of arguments. That is, formatting has to be supported by xlsx exporter like excel exporter.

Not 100% sure. Yep, it looks humane, but if/when you'll add formatting support for Excel exporters -- conventions on formatting are pretty different in two versions of Excel files and corresponding libraries.
In fact, I believe that in 2017 support for Excel 1999-2003 format (xls) should be done in "maintenance" mode, while xlsx exporter should be shiny and friendly.

zverok · 2017-10-30T19:12:46Z

README.md

+    string = df.to_excelx_string(index: false)
+    df.write_excel('path/to/file.xlsx', index: false)
+    ```
+


I am starting to think this part of README becames too long and repetitive. Just as an idea for the future:

Leave 2-3 most "tasty" examples in REDME + link to...

Formats.md with regular structure...

Which is built automatically, gathering the comments from top of all exporters/importers files, with similar format.

WDYT?

For generating files like {FORMAT}_Importer.md, does an ERB template work? Or is there a better option?

Let's think. In my head, it (probably) does the following:

takes all the importers (exporters) code;

extract, say, top-of-the-file comment with explanations;

join them together with pretty headers;

store to Formats.md (one, not document-per-formatter, it is tiresome to browse).

probably generates TOC on the top of the file;

probably adds some "preface" at the top of the file (taken from some _preface.md).

Can be implemented with just Ruby code, or small ERB template, which is the simplest.

WDYT?

@zverok - Acknowledged. Let us take this discussion to #71 please? 😄

zverok · 2017-10-30T19:13:27Z

lib/daru/io/exporters/csv.rb

@@ -80,6 +80,7 @@ def write(path)
            contents.each { |content| csv << content }
            csv.close
          end
+          true


Why is it necessary?

I thought that this could act as a consistent means of checking whether the "writing into a file" actually took place. Else, the File.write(...) just returns the number of characters written - which may not mean much to the user/developer compared to a consistent (constant) true.

It's not really necessary, but I thought this would be better. Let me know if this can be reverted.

zverok · 2017-10-30T19:15:51Z

lib/daru/io/exporters/excelx.rb

+      # `Daru::DataFrame` instance variables
+      class Excelx < Base
+        Daru::DataFrame.register_io_module :to_excelx_string, self
+        Daru::DataFrame.register_io_module :write_excelx, self


Not 100% sure. Yep, it looks humane, but if/when you'll add formatting support for Excel exporters -- conventions on formatting are pretty different in two versions of Excel files and corresponding libraries.
In fact, I believe that in 2017 support for Excel 1999-2003 format (xls) should be done in "maintenance" mode, while xlsx exporter should be shiny and friendly.

zverok · 2017-10-30T19:17:52Z

lib/daru/io/exporters/excelx.rb

+          super(file_extension: '.xlsx')
+        end
+
+        # Exports an Excelx Exporter instance to an xlsx file.


I believe it doesn't export the exporter, it "exports the dataframe", or "performs the export(er)".

(If it is standard phrase for all exporters, this notice is related to all of them).

zverok · 2017-10-30T19:18:15Z

lib/daru/io/exporters/excelx.rb

+        # @param path [String] Path of excelx file where the dataframe is to be saved
+        #
+        # @example Writing an Excelx Exporter instance to an xlsx file
+        #   instance.write('filename.xlsx')


I don't believe that such a simple example brings any clarity. Just remove it?

zverok · 2017-10-30T19:19:27Z

lib/daru/io/exporters/excelx.rb

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


Is this or similar statement repeated in other places in codebase (calculating of index witdth)? Maybe move it to some utility module, or to base exporter?

zverok · 2017-10-30T19:20:23Z

lib/daru/io/exporters/excelx.rb

+          @col_offset ||= @dataframe.index.is_a?(Daru::MultiIndex) ? @dataframe.index.width : 1
+        end
+
+        def fetch_headers


Is "fetch" a good name? "Fetch" implies some external source to be used, while this method in fact "formats" or "renders" its own instance variables.

zverok · 2017-10-30T19:21:39Z

lib/daru/io/exporters/excelx.rb

+          case idx
+          when Daru::Vector, Daru::MultiIndex, Array then idx.map(&:to_s)
+          else [idx.to_s]
+          end


Wouldn't Array(idx).map(&:to_s) work instead of this case?..

zverok · 2017-10-30T19:24:56Z

lib/daru/io/exporters/excelx.rb

+          @data   = data
+          @index  = index
+          @sheet  = sheet
+          @header = header


Hm, do we have this lot of boolean vars in other importers too? I believe that one hash @render_parts = {data: data, headers: headers, index: index} could be more effective and will allow to generalize a lot. In addition, looking at var names you can wrongly conclude that, say, @index contains dataframe's index, not "if we need to render index" boolean.

zverok · 2017-10-30T19:35:32Z

lib/daru/io/exporters/excelx.rb

+          formatting(row, @data)
+        end
+
+        def formatting(idx, format)


Not the best name, as we'll add "real" Excel formatting once. Something like just to_a?.. Like this:

# or "to_strings"?.. def to_a(object, render = true) return [] unless render Array(object).map(&:to_s) end

BTW, have you checked that to_s is good enough? For example, wouldn't numbers or dates be formatted as strings, instead of appropriate Excel column types?

athityakumar · 2017-12-07T10:25:56Z

Oops, I didn't notice that this Pull Request had already been reviewed. I'll soon push the changes for the next review. Meanwhile, I've also opened #71 for keeping track of, in a different PR.

athityakumar added 7 commits October 24, 2017 22:09

Adds excelx exporter with axlsx dependency

13e71b9

Changes source of axlsx from rubygems to git

aafde94

Re-writes excelx exporter with rubyXL gem

1035f6c

Updates YARD Doc for Excelx Exporter

01fc93f

Updates README with Excelx Exporter

a6746c4

Adds tests for Excelx Exporter

f478470

Fixes minor rubocop offense

2f2bd6b

athityakumar added the Exporter label Oct 25, 2017

athityakumar requested a review from zverok October 25, 2017 21:00

athityakumar added the in progress label Oct 25, 2017

athityakumar commented Oct 25, 2017

View reviewed changes

zverok reviewed Oct 30, 2017

View reviewed changes

athityakumar mentioned this pull request Dec 7, 2017

Auto-generate Importer-Exporter markdown templates #71

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for Excelx (xlsx) Exporter #64

Adds support for Excelx (xlsx) Exporter #64

athityakumar commented Oct 25, 2017 •

edited

Loading

athityakumar Oct 25, 2017

zverok Oct 30, 2017

zverok Oct 30, 2017

athityakumar Nov 16, 2017

zverok Nov 16, 2017

athityakumar Dec 7, 2017

zverok Oct 30, 2017

athityakumar Dec 7, 2017

zverok Oct 30, 2017

zverok Oct 30, 2017

zverok Oct 30, 2017

zverok Oct 30, 2017

zverok Oct 30, 2017

zverok Oct 30, 2017

zverok Oct 30, 2017

zverok Oct 30, 2017

athityakumar commented Dec 7, 2017

Adds support for Excelx (xlsx) Exporter #64

Are you sure you want to change the base?

Adds support for Excelx (xlsx) Exporter #64

Conversation

athityakumar commented Oct 25, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athityakumar commented Dec 7, 2017

athityakumar commented Oct 25, 2017 •

edited

Loading