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

[R/wbStyle] remove the remaining parts of wbStyle #103

Merged
merged 2 commits into from
Mar 23, 2022
Merged

Conversation

JanMarvin
Copy link
Owner

It's highly unfortunate that so much of your time went into tweaking and cleaning this @jmbarbone , but hopefully I've told you early enough that it had no future. This thing is a monster that was awfully complex and the benefits of having it in openxlsx are getting fewer and fewer.

Header styling is no longer possible (it's possible working with wbWorkbook, but not in write.xlsx). Borders are possible, but they are lacking a user facing function, right now the user would have to create for each border cell the required border and cell style. Once we create a new add_style function, that creates a cell style swiftly and with style, the entire conversion should be done. Right now I just wanted to clean up the code and avoid having other users playing around with this, even though it is gone.

@JanMarvin
Copy link
Owner Author

JanMarvin commented Mar 22, 2022

Codecov Report

Merging #103 (3ac7fc9) into main (3b6ca04) will increase coverage by 4.64%.
The diff coverage is 9.67%.

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   51.34%   55.98%   +4.64%     
==========================================
  Files          42       40       -2     
  Lines        8419     7498     -921     
==========================================
- Hits         4323     4198     -125     
+ Misses       4096     3300     -796     
Impacted Files Coverage Δ
R/asserts.R 56.25% <ø> (-11.10%) ⬇️
R/class-comment.R 77.08% <ø> (ø)
R/class-workbook-wrappers.R 31.60% <ø> (+0.53%) ⬆️
R/class-workbook.R 50.34% <ø> (+8.98%) ⬆️
R/conditional_formatting.R 0.00% <ø> (ø)
R/helperFunctions.R 78.15% <ø> (+51.80%) ⬆️
R/wb_styles.R 33.94% <0.00%> (-3.87%) ⬇️
R/writeData.R 76.13% <ø> (+0.58%) ⬆️
R/writeDataTable.R 75.00% <ø> (+6.25%) ⬆️
R/writexlsx.R 33.19% <ø> (+0.11%) ⬆️
... and 4 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 3b6ca04...3ac7fc9. Read the comment docs.

@jmbarbone
Copy link
Collaborator

😢

But at least it gives us room to completely redo adding/creating styles. Ground-up redo will gives us a lot more room. The formatting is definitely one of openxlsx's strongest advantages but the backend left something to be desired. I just prettied up some of it -- so no a lot lost there/

And this increases our code coverage!

@JanMarvin
Copy link
Owner Author

😄 well fortunately I still know a few places where we could increase our coverage that do not require removing 3k loc!

Style objects are nice, I do not try to invalidate this argument, but they were constructed inefficient and hard to improve. Fortunately a lot of the internal handling is now already covered: conditional formatting, comment styles and number formats.

For cell styling there are already examples in the style manager and in the style vignette. We already have a lot covered, but it's ofc still a bit edgy to use and some friendly add_style() easing the burden is still required.

Borders are currently the thing frightening me the most (they require a lot of tedious cell construction and that usually isn't fun. There are reasons, why even in Excel the handling is no fun.

@JanMarvin JanMarvin merged commit 8d66780 into main Mar 23, 2022
@JanMarvin JanMarvin deleted the rm_wb_styles branch March 23, 2022 07:02
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