-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add rudimentary support for hyperlinks in cells #63
base: master
Are you sure you want to change the base?
Conversation
This is rough and ready but works for me with the file I have
Let me know when you have a test for this - I'd rather not add new features that I'm not familiar with without tests (so that I don't accidentally break them in the future). |
This is based upon TestActiveSheet.xls in the Spreadsheet::ParseExcel distribution
This is based upon test 47_hyperlinks.t in Spreadsheet::ParseExcel
Here I was trying to match a return value in the Spreadsheet::ParseExcel test but actually lets just make this a todo test for now as otherwise I'm potentially opening up to pitfalls
Per previous commit stop trying to frig values without really understanding what's going on
Match other test files
They are ok on my machine but the version on github seems to have \r in it
Hi, Tweaked the code to make it slightly less rudimentary (so it now handles internal links as well as external) and to work with the same set of tests as Spreadsheet::ParseExcel. I've marked some of the tests as TODO as I get back different values to Spreadsheet::ParseExcel - to my mind the values returned by this module are correct as they are what is in the spreadsheet, but it does differ to what Spreadsheet::ParseExcel was expecting, so I've left them expecting the values Spreadsheet::ParseExcel was for now rather than changing to what this module actually returns. Thanks |
Per #97, please create a PR at https://github.com/r-gregmisc/spreadsheet-parsexlsx. |
This is rough and ready but works for me with the file I have.
I don't currently have time to add a test but will try to do so in the next few weeks.