search

roo-rb / roo

Roo provides an interface to spreadsheets of several sorts.
MIT License
2.79k stars 501 forks source link

Allows sheet_for to accept an integer #455

Closed tgturner closed 6 years ago

tgturner commented 6 years ago

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: https://github.com/roo-rb/roo/issues/447

coveralls commented 6 years ago

Coverage Status

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

tgturner commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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

tgturner commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

I was thinking about former. LGTM