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

ReferenceClass cleanup #14

Merged
merged 61 commits into from
Dec 31, 2021
Merged

ReferenceClass cleanup #14

merged 61 commits into from
Dec 31, 2021

Conversation

jmbarbone
Copy link
Collaborator

⚠️ Substantial changes ⚠️

This will be one of several merges of main <- rc-cleanup. This will probably have the largest changes (see below) so I wanted to get this merged before getting into other small clean-up tasks throughout the package.

It's not going to be feasible to review everything (128 files and 8000+ lines?!) but all the tests work and these changes should make it easier to debug future failures. Most of the changes are not even functional.

Short summary

  • files have been renamed: Organizes the R scripts with class-object
  • large code chunks moved around
    • Each class and it's fields/methods are defined in a single setRefClass() rather than scatter throughout one or more files (this also removes the Collate field in DESCRIPTION so we don't need a specific file order to work!)
    • Some wrapper functionality have been moved inside the classes (e.g., createStyle() had many checks that have been moved into Style$new(), figuring that initiating the object should also validate the inputs); there are still more to be done
  • many changes made inside the RC objects
    • e.g., using .self$object <- value rather than object <<- value
    • a bit of a preference but makes it very obvious when fields are being altered
    • it's a little slower but the intended changes to R6 will make up for the small increase

@jmbarbone
Copy link
Collaborator Author

@JanMarvin looking at the commit history on main I don't think I saw anything that wouldn't be carried over (except for a89ac74, which I just added)

If there were updates to the any of the RC classes they may not have been carried over because of the file name changes. Let me know know if there were any I missed and I can reimplement them.

@JanMarvin
Copy link
Owner

Thanks, that's huge! I'll try to look at it tomorrow and hope to get it merged soon.

@JanMarvin
Copy link
Owner

Seeing this:

"Some wrapper functionality have been moved inside the classes (e.g., createStyle() had many checks that have been moved into Style$new(), figuring that initiating the object should also validate the inputs); there are still more to be done"

Just to avoid unnecessary work, I highly appreciate the work you do in the cleaning process, but cannot guarantee that the internal functions will look the same. In #10 I began swinging the axe at the styleObject upon which many internal functions rely on. Borders, fonts, fills etc. (In my opinion the creation and handling should be reimagined, but aside from the obvious styles.xml file not everything will remain intact.) Even though this is still a draft in early stages (and maybe I'll have to revert everything in two months, trying brings the option of failing), please don't spend lots of time improving the old code regarding these matters. Getting familiar with the logic ofc is more than welcome.

@jmbarbone
Copy link
Collaborator Author

No worries. The Style stuff is bad luck on my end. I sort of randomly chose that to test some things and then realized later you were doing some things in #10. But that work looks like it was on the Workbook style functions, not the Style class. It could still be useful for creating custom styles, since it contains plenty of checks, and could then then be extracted/merged into the simple structure that #10 seems to be aiming towards.

I think most of my changes are really to test the package logic. Break apart some things, move them around, see if it still works. More untangling than improving so we can be sure we're not removing anything terribly important and make it easier to see where bigger improvements can be made.

@JanMarvin
Copy link
Owner

Thanks, I merge as is

@JanMarvin JanMarvin merged commit d413960 into main Dec 31, 2021
@jmbarbone jmbarbone deleted the rc-cleanup branch May 5, 2022 14:59
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.

None yet

2 participants