Allows sheet_for to accept an integer #455

tgturner · 2018-09-17T17:48:56Z

Currently sheet_for does not allow an index for the sheet to be passed. This fix maintains compatibility by checking if a name is passed first, and then checking if the sheet's index is passed.

Fixes some bugs noticed here: #447

coveralls · 2018-09-17T17:57:59Z

Coverage increased (+0.8%) to 94.581% when pulling c1d6501 on tgturner:bugfix/sheet_for-with-index into bd0b0f8 on roo-rb:master.

chopraanmol1

Additional TODO:
Add example for sheet_for with integer based index in #451

chopraanmol1 · 2018-09-18T05:07:21Z

lib/roo/excelx.rb

@@ -97,7 +97,7 @@ def sheets
    def sheet_for(sheet)
      sheet ||= default_sheet
      validate_sheet!(sheet)
-      @sheets_by_name[sheet]
+      @sheets_by_name[sheet] || @sheets[sheet - 1]


I understand the reason behind using (sheet - 1) is to keep things consistent with validate_sheet!, which is a good thing.

But I think it is confusing to have two methods (sheet, sheet_for) with different starting index. i.e. sheet => 0, sheet_for => 1

What about checking the type of sheet parameter before validate_sheet! and manually adjust sheet value (sheet + 1) in case of a integer. This will keep things consistent to both validate_sheet! and sheet.

In long-term we can somehow warn in depreciation warning that we are changing starting index for sheet and sheet_for from 0 to 1, or maybe we won't need this.

I thought about this exact issue. My issue with having this being 0 indexed is that nearly every other method relies on being 1 indexed, in regards to sheets, rows, and columns.

My longterm goal here is to actually deprecate the sheet method in its current form. It causes issues because the API is very different for Excel vs. Other. Our implementation of Excel actually has the concept of Sheets, which are objects that can be returned. Every other format we suppourt doesn't actually have a sheet object. I would like to settle on one implementation, either ALWAYS use Sheet objects, or hide sheet objects and only operate on the Top level instance, ie. Excel, OpenOffice, CSV etc.

What are you thoughts?

tgturner · 2018-09-18T18:22:12Z

I'm starting to disagree with adding sheet_for to readme at all right now. Excel is currently the only format that supports that method. And the concept of returning a Sheet instance only makes sense for excel.

My opinion would be for this to go in as is, and for us to come to a decision about what to do with the concept of "sheets". Are you alright with that?

tgturner · 2018-09-19T18:53:40Z

@chopraanmol1

Also, side note, using default_sheet= also expects 1 indexed sheets, not 0 indexed, so the sheet method really is the one misbehaving here

chopraanmol1 · 2018-09-20T05:30:33Z

@tgturner I tried using default_sheet= with integer and it failed for both Roo::OpenOffice and Roo::Excelx. Only sheet method worked with integer value among the limited no. of method I tested.

For Excelx most of the methods use sheet_for method for determining sheet which does not yet work with Integer value as an argument. Can you give provide a list of methods which works with 1 indexed sheet?

Note: validate_sheet! methods is used in 3 different methods Base#default_sheet=, Excelx#sheet_for, OpenOffice#read_cells and none of them (& dependent methods) handles Integer based index properly

tgturner · 2018-09-20T15:44:23Z

@chopraanmol1 You are correct. I saw the comment above default_sheet= and assumed it worked that way. Since no places that were using validate_sheet! were actually accepting integers, I changed it around.

I also decided to have default_sheet= still set default_sheet with the string name to avoid any trouble having default_sheet be an integer might cause us down stream.

chopraanmol1 · 2018-09-21T05:01:01Z

test.rb

+    # end
+    # puts Benchmark.measure{ Roo::Excelx.new('test/files/Bibelbund.xlsx').tap{|x|(2..x.last_row).each{|i| x.row(i)}} }
+  end
+end


Care to remove this file?

Great catch! Sorry for that!

chopraanmol1

Can you add more test case?

For non-first sheet
With Invalid sheet name and index

rest looks good to me.

sheet_for and default_sheet= can now accept the sheet as integer and as a string.

tgturner · 2018-09-21T15:04:26Z

@chopraanmol1 Thanks for the feedback!

I added a couple more test cases and squashed the commits. Let me know if you were thinking that the test cases should go somewhere else or if you were looking for something different.

chopraanmol1 · 2018-09-21T15:11:53Z

@tgturner new test cases looks good. It will be good to have same spec covered for sheet_for. And I think we also need to handle Roo::OpenOffice#read_cells.

After this we can go ahead.

tgturner · 2018-09-21T15:21:29Z

@chopraanmol1 Added spec for sheet_for.

This change will not impact Roo::OpenOffice#read_cells because default_sheet will still always be a string, not an integer. Unless there is something else you were thinking we needed to add tests for with read_cells.

chopraanmol1 · 2018-09-21T15:24:50Z

I was thinking about former. LGTM

pkgsrc change: add "USE_LANGUAGES= # none". ## [2.8.0] 2019-01-18 ### Fixed - Fixed inconsistent column length for CSV [375](roo-rb/roo#375) - Fixed formatted_value with `%` for Excelx [416](roo-rb/roo#416) - Improved Memory consumption and performance [434](roo-rb/roo#434) [449](roo-rb/roo#449) [454](roo-rb/roo#454) [456](roo-rb/roo#456) [458](roo-rb/roo#458) [462](roo-rb/roo#462) [466](roo-rb/roo#466) - Accept both Transitional and Strict Type for Excelx's worksheets [441](roo-rb/roo#441) - Fixed ruby warnings [442](roo-rb/roo#442) [476](roo-rb/roo#476) - Restore support for URL as file identifier for CSV [462](roo-rb/roo#462) - Fixed missing location for Excelx's links [482](roo-rb/roo#482) ### Changed / Added - Drop support for ruby 2.2.x and lower - Updated rubyzip version for fixing security issue. Now minimal version is 1.2.1 - Roo::Excelx::Coordinate now inherits Array [458](roo-rb/roo#458) - Improved Roo::HeaderRowNotFoundError exception's message [461](roo-rb/roo#461) - Added `empty_cell` option which by default disable allocation for Roo::Excelx::Cell::Empty [464](roo-rb/roo#464) - Added support for variable number of decimals for Excelx's formatted_value [387](roo-rb/roo#387) - Added `disable_html_injection` option to disable html injection for shared string in `Roo::Excelx` [392](roo-rb/roo#392) - Added image extraction for Excelx [414](roo-rb/roo#414) [397](roo-rb/roo#397) - Added support for `1e6` as scientific notation for Excelx [433](roo-rb/roo#433) - Added support for Integer as 0 based index for Excelx's `sheet_for` [455](roo-rb/roo#455) - Extended `no_hyperlinks` option for non streaming Excelx methods [459](roo-rb/roo#459) - Added `empty_cell` option to disable Roo::Excelx::Cell::Empty allocation for Excelx [464](roo-rb/roo#464) - Added support for Integer with leading zero for Roo:Excelx [479](roo-rb/roo#479) - Refactored Excelx code [453](roo-rb/roo#453) [477](roo-rb/roo#477) [483](roo-rb/roo#483) [484](roo-rb/roo#484) ### Deprecations - Roo::Excelx::Sheet#present_cells is deprecated [454](roo-rb/roo#454) - Roo::Utils.split_coordinate is deprecated [458](roo-rb/roo#458) - Roo::Excelx::Cell::Base#link is deprecated [457](roo-rb/roo#457)

* Allows sheet_for and default_sheet= to accept int sheet_for and default_sheet= can now accept the sheet as integer and as a string. * Adds spec for sheet_for

tgturner requested a review from chopraanmol1 September 17, 2018 17:48

chopraanmol1 requested changes Sep 18, 2018

View reviewed changes

chopraanmol1 reviewed Sep 21, 2018

View reviewed changes

chopraanmol1 requested changes Sep 21, 2018

View reviewed changes

Allows sheet_for and default_sheet= to accept int

935c9c7

sheet_for and default_sheet= can now accept the sheet as integer and as a string.

tgturner force-pushed the bugfix/sheet_for-with-index branch from 8837259 to 935c9c7 Compare September 21, 2018 14:56

Adds spec for sheet_for

c1d6501

chopraanmol1 approved these changes Sep 21, 2018

View reviewed changes

chopraanmol1 merged commit bfcfba4 into roo-rb:master Sep 21, 2018

tgturner deleted the bugfix/sheet_for-with-index branch September 21, 2018 16:23

GeoffAbtion mentioned this pull request Jan 22, 2020

Document sheet_for in README #451

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows sheet_for to accept an integer #455

Allows sheet_for to accept an integer #455

tgturner commented Sep 17, 2018

coveralls commented Sep 17, 2018 •

edited

Loading

chopraanmol1 left a comment

chopraanmol1 Sep 18, 2018

tgturner Sep 18, 2018

tgturner commented Sep 18, 2018 •

edited

Loading

tgturner commented Sep 19, 2018

chopraanmol1 commented Sep 20, 2018

tgturner commented Sep 20, 2018

chopraanmol1 Sep 21, 2018 •

edited

Loading

tgturner Sep 21, 2018

chopraanmol1 left a comment

tgturner commented Sep 21, 2018

chopraanmol1 commented Sep 21, 2018

tgturner commented Sep 21, 2018

chopraanmol1 commented Sep 21, 2018 •

edited

Loading

Allows sheet_for to accept an integer #455

Allows sheet_for to accept an integer #455

Conversation

tgturner commented Sep 17, 2018

coveralls commented Sep 17, 2018 • edited Loading

chopraanmol1 left a comment

Choose a reason for hiding this comment

chopraanmol1 Sep 18, 2018

Choose a reason for hiding this comment

tgturner Sep 18, 2018

Choose a reason for hiding this comment

tgturner commented Sep 18, 2018 • edited Loading

tgturner commented Sep 19, 2018

chopraanmol1 commented Sep 20, 2018

tgturner commented Sep 20, 2018

chopraanmol1 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

tgturner Sep 21, 2018

Choose a reason for hiding this comment

chopraanmol1 left a comment

Choose a reason for hiding this comment

tgturner commented Sep 21, 2018

chopraanmol1 commented Sep 21, 2018

tgturner commented Sep 21, 2018

chopraanmol1 commented Sep 21, 2018 • edited Loading

coveralls commented Sep 17, 2018 •

edited

Loading

tgturner commented Sep 18, 2018 •

edited

Loading

chopraanmol1 Sep 21, 2018 •

edited

Loading

chopraanmol1 commented Sep 21, 2018 •

edited

Loading