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.

Sign up for GitHub

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

Jump to bottom

internal R6 workbook implementation functions #396

Open
jmbarbone opened this issue Oct 29, 2022 · 3 comments · May be fixed by #471
Open

internal R6 workbook implementation functions #396

jmbarbone opened this issue Oct 29, 2022 · 3 comments · May be fixed by #471
Assignees
@jmbarbone
Labels
internal 🔧 R6 6️⃣

Comments

@jmbarbone
Copy link
Collaborator

R/class-workbook.R has become quite massive (6000+ lines!)

This can be reduced by a little {R6} wrapper trick (yes, more wrappers). An example can be seen with the {desc} package: https://github.com/r-lib/desc/blob/main/R/description.R

method inside object:

set_value = function(wb, ...) {
  set_value_impl(self, private, ...)
}

function outside object:

set_value_impl <- function(self, private, ...) {
  # perform some actions
  ...
  self
}

Because self and private are environments, they can be modified wherever. This means we'd be able to pull these values into separate functions and save some of the more complex functions in separate files.

Essentially,

wb_function(wb, ...) calls
wb$function(...) calls
wb_function_impl(self, private, ...)
@jmbarbone jmbarbone self-assigned this Oct 29, 2022
@jmbarbone
Copy link
Collaborator Author

@JanMarvin , if we have some other heavy lifting around the wbWorkbook class, I can try to knock this out in a weekend. It would present a lot of changes that would be hard/annoying to merge with any other development.

@JanMarvin
Copy link
Owner

Don't recall anything urgent from the top of my head regarding wbWorkbook. I was thinking about exporting a couple of wbWorkbook functions to individual functions similar to write_data() and write_data_table(). But if there are other options let's go for it. After 0.3.1 there are still enough things to work on :)

@jmbarbone
Copy link
Collaborator Author

There's some a dozen or so pending R6 issues that I haven't gotten to, so hopefully this sort of cleanup will make those a bit easier to manage. I don't think this will have any visible user changes, and should be pretty safe, too, unless I mess up.

@jmbarbone jmbarbone linked a pull request Dec 13, 2022 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jmbarbone jmbarbone

Labels
internal 🔧 R6 6️⃣
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

396 R6 impl JanMarvin/openxlsx2
2 participants
@JanMarvin @jmbarbone