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

R6 classes #47

Merged
merged 16 commits into from
Jan 23, 2022
Merged

R6 classes #47

merged 16 commits into from
Jan 23, 2022

Conversation

jmbarbone
Copy link
Collaborator

@jmbarbone jmbarbone commented Jan 23, 2022

closes #40

The workbook modifications might look a little confusing because I moved some functions, so the line changes may not look entirely correct.

I've also updated the names to use a prefix of wb to avoid any unwanted conflicts (wbHyperlink seemed safer than just Hyperlink) and updated the new wrappers accordingly (e.g., new_workbook(), new_comment() to wb_workbook(), wb_comment()). As an FYI, the package name workbook is available on CRAN, just sayin'.

For the most part, all of the methods from the ReferenceClass object remain public. Only in the wbWorkbook class did I move anything to private because it was only used inside a public method. There are some functions that weren't used at all that I moved into private as well -- not sure if we'll be keeping them or not.

The documentation is also going to need a big overhaul. Most of it is the bare minimum (to prevent complaints in check) and I didn't want to spend too much time figuring out what everything is if we're going to be be removing/revamping a lot of it. I think we can probably just try our best to update the documentation on the go while we made other edits.

There are some additional changes outside of just converting from ReferenceClass to R6 that I made after finding some errors/warnings in the check()

Plenty of other clean up to do (see #9) but I think this is a good start.

* change a few checks for cc as uninitializedField to NULL
* update C++ loadvals() to use Environment (R6) rather than Reference
* add more missing `self$`
* some cleanup in other files from debugging
* functional methods no in use moved to private()
* add missed self$
* minimal documentation added (some of these may be removed anyway
* replace Workbook inherit checks with assert_workbook()
* update workbook methods for names()
* add missing self$
* update class checks with assert_*
* conversion of "NaN" for excel "$NUM!"
@JanMarvin
Copy link
Owner

Codecov Report

Merging #47 (9723d22) into main (3c887a5) will increase coverage by 0.13%.
The diff coverage is 48.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
+ Coverage   35.67%   35.81%   +0.13%     
==========================================
  Files          42       42              
  Lines       10536    10494      -42     
==========================================
- Hits         3759     3758       -1     
+ Misses       6777     6736      -41     
Impacted Files Coverage Δ
R/class-workbook.R 26.60% <ø> (+0.23%) ⬆️
R/conditional_formatting.R 0.00% <0.00%> (ø)
R/wb_styles.R 5.57% <0.00%> (ø)
R/writeData.R 58.41% <0.00%> (-1.00%) ⬇️
R/writeDataTable.R 72.09% <0.00%> (-1.17%) ⬇️
src/styles_xml.cpp 11.91% <0.00%> (ø)
src/helper_functions.cpp 27.06% <18.18%> (ø)
R/class-style.R 44.57% <31.03%> (+0.17%) ⬆️
R/class-sheet-data.R 39.58% <40.27%> (+6.25%) ⬆️
R/class-comment.R 10.52% <47.05%> (+0.13%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c887a5...9723d22. Read the comment docs.

@JanMarvin JanMarvin merged commit c71c905 into main Jan 23, 2022
@JanMarvin
Copy link
Owner

Thanks so much! Will go through the changes, currently just merged. Regarding the documentation, I agreed that we write it once we're reach that bridge.

z[nums] <- lapply(z[nums], as.numeric)
# convert "#NUM!" to "NaN" -- then converts to NaN
# maybe consider this an option to instead return NA?
z[nums] <- lapply(z[nums], function(i) as.numeric(replace(i, i == "#NUM!", "NaN")))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want and will add an option here. It might not be useful for R users, but for people looking for bugs in Excel files. Previously openxlsx and all the other packages dropped this content, similar to formulas, and if you're not working with Excel but receive a sheet and try to understand what error your colleagues mean, this could be useful.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meaning: If you see NaN it might be some kind of conversion gone wrong on the R side. Seeing #NUM gives an indication that there is a formula that was badly evaluated, when the file was written.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move to R6
2 participants