ENH Pass kwds from ExcelFile ctr to xlrd.open_workbook. For example, thi...
State: closed

...s allows setting  formatting_info=True (GH4438)

closes #4438


Labels:

Comments:
@cancan101 Please write a test! :smile: I find it very helpful to write a test _before_ I make a change (a hard habit to get into especially if you think the issue/change is small, but it will help you in the long run)

Also please add some documentation showing users that you can now pass keyword arguments when reading excel files.

@cpcloud that make sense. I will do. That being said, should I rename the parameter on ExcelFile to be something more informative and less specific to xlrd? perhaps "find_merged_cells" as opposed to formatting info ? That way parameter can be kept the same if underlying library changes.

@cpcloud Unit test pushed

@cpcloud @jreback Other than release notes, anything else I should do to get this merged?

Add a note to release.rst and squash this into one commit and I'll try to review in the interim. 

This goes back to the question about `xlrd` dependence. I think my commit is still valid, but do we want to add an additional parameter to `ExcelFile`, something like `load_merged_cells` that is not specific to `xlrd`, which `formatting_info=True` is.

That way if `xlrd` is dropped as the Excel file reader, `load_merged_cells` can be mapped to the new library?

Or should `formatting_info=True` be on by default given the plans to support `MultiIndex`?

To be honest, we could probably hide this entire parameter from the user. If we see a need to load merged cells, such as when `header` is set to a be an iterable of length > 1, we could pass `formatting_info=True` to `xlrd` and then use `get_effective_cell` (see: #4673) to read the cells.

Thoughts?

Couldn't the test case for this just check that merged cells are produced? Does the content matter (seems like it's just what xlrd does)? 

@jtratner I am not sure what you mean.

It's fine to pass keywords through (and that's reasonable), but 'getting the value of merged cells' is probably not something pandas should be set to do. At that point, you could just use xlrd directly.

To be clear I mean: converting merged cells into something reasonable in the outputted dataframe makes sense. It doesn't make sense to add something to the public api targeted specifically at merged cells. 

@jtratner I am still not sure what you mean. Maybe I was unclear with my comment: I was just saying there might be a smart way of allowing us to parse merged cells in the ExcelParser without the user having to specify `formatting_info=True`

@jtratner I think we are in agreement. When I said "hide the parameter" what I should have said was not "not add the parameter to the public API"

@cancan101 a few final things to get this accepted:
1. please revise your test case to be shorter (i.e., just check that merged_cells is there and has length 1 - the rest is just an unnecessary smoke test for xlrd).
2. Please add a note to release.rst documenting what you've changed
3. Add a note to the docstring for `ExcelFile` that specifies that `kwds` are passed directly to the `xlrd` constructor. I think you should also note that it's not guaranteed that pandas will continue to use `xlrd` in the future so this is discouraged. - not sure - what do you think @jreback @cpcloud ?
4. Please squash this down to 1 commit.

do u think have a flavor (or maybe engine?) argument is useful here (to specify the backend) , eg would be 'xlrd' in this case (so we can offer back compat at some point if needed), or overkill ?

@jreback I think there are already PRs for using other readers/writers for Excel, so not necessarily a bad thing to have. I just don't want to be tied to supporting any keyword arguments that aren't part of the public API.

@jtratner is this obsolete? (as in your new framework already does this)?

no, still up for grabs as to what to do (didn't refactor ExcelFile)

gr8, then is PR look ok?

@jtratner What is the best way to make this future proof to changes in the Excel parser (ie moving away from xlrd)?

@cancan101 I think it would be better to just allow `read_excel` to accept workbooks too. That way, you can do whatever  you want to the table first and there's no requirement that read_excel itself expose things that aren't directly necessary for creating pandas objects.

@jtratner As a slight modification to your suggestion: 

Change `ExcelFile` to accept a workbook for the `path_or_buf` argument. That will also mean that `read_excel` accepts workbooks too.

yep, that's what I was thinking.

Closing in favor of adding an option to let ExcelFile take an existing workbook.

@jtratner Can you leave this PR open? I was about to push fix / change to allow passing in sheet.

you should create a new PR for that, the discussion on this was about passing kwargs...confusing to do both. Plus, there was the additional discussion about the test cases.

btw - this is why it's good to do topic branches instead of directly on your own master. Lets you have multiple PRs open.

Do you mind if I leave a test case that tests both:

```
    def test_excel_read_merged_cells(self):
        _skip_if_no_xlrd()

        import xlrd

        pth = os.path.join(self.dirpath, 'merged.xls')
        xls = ExcelFile(xlrd.open_workbook(pth, formatting_info=True))        
        book = xls.book
        sheet = book.sheet_by_index(0)
        merged_cells = sheet.merged_cells

        self.assertEquals(len(merged_cells), 1)
```

@jtratner Yea, using master was a mistake. I usually use topic branches. Once pushed, github does not allow changing branch for PR.