Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extract worksheet reading code from functions #7

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented May 18, 2016

This is not a genuine PR @richfitz. I've created a faux vignette, where I unpack all the code necessary to read the data from a workbook with one sheet. I propose this PR/vignette as a way to have a concrete conversation as I figure out how rexcel works. Eventually we could comment around specific lines. But first, can I get your high-level reaction?

  1. Does this seem a decent way to proceed?
  2. It feels like rexcel's reading could be made simpler, but of course I don't have specific proposals yet! Is that also your sense or do you think there aren't any major simplifications available?
  3. I do still want you to look after Excel, while I look after Google. So I'm trying to not get in too deep here. But I think I have to get some understanding of rexcel, because it's driven the current specs of linen. This is more a comment than question.

@richfitz
Copy link
Member

Nice! I have more packages you could do this to if you'd like 😀

readxl's reading could be made simpler, but for my medium term sanity the main thing I'd like to keep is that most of the low-level stuff mirrors the names in the spec (all the functions with ct and st in them). I can comb through and see if it can be cleaned up, but the main thing that I know is needed is some further quarantining of style code.

I'll try and get through the actual vignette soon

@jennybc jennybc mentioned this pull request May 18, 2016
@jennybc
Copy link
Member Author

jennybc commented May 18, 2016

I think quarantining of the style code could be achieved at the same time as my suggestion to create a workbook registration function (that treats all worksheets equally and gathers a bit of info on them), should you choose to accept it.

jennybc added 26 commits May 18, 2016 12:08
This is also me just exploring all the files in the unzipped xlsx. And then exposing anything that looks valuable. I'm sure this has lots of overlap with existing linen::workbook.
similar to xlsx_read_workbook() but does less: only reads from the single file xl/workbook.xml
# Conflicts:
#	DESCRIPTION
so I need to be more specific here
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants