ycphs / openxlsx

openxlsx - a fast way to read and write complex xslx files
https://ycphs.github.io/openxlsx/
Other
224 stars 75 forks source link

Add auto row_heights feature #375

Closed JanMarvin closed 2 years ago

JanMarvin commented 2 years ago

Fixed code by @DavidBreuer pushed by @David-Rattray in #373.

codecov-commenter commented 2 years ago

Codecov Report

Merging #375 (b98727c) into development (97e0fe3) will increase coverage by 0.01%. The diff coverage is 76.19%.

@@               Coverage Diff               @@
##           development     #375      +/-   ##
===============================================
+ Coverage        70.32%   70.34%   +0.01%     
===============================================
  Files               34       34              
  Lines             9005     9041      +36     
===============================================
+ Hits              6333     6360      +27     
- Misses            2672     2681       +9     
Impacted Files Coverage Δ
R/loadWorkbook.R 84.06% <ø> (-0.10%) :arrow_down:
R/helperFunctions.R 88.39% <65.51%> (-1.44%) :arrow_down:
R/wrappers.R 57.85% <100.00%> (+0.35%) :arrow_up:

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 5757de3...b98727c. Read the comment docs.

David-Rattray commented 2 years ago

Hi @JanMarvin been busy/unwell for awhile, but aiming to get back on this. As you mentioned in the closed pull request, I'll look to get feature tests and backward testing sorted out, and along the way maybe clean things up a smidgeon. I'm going to just read https://r-pkgs.org/testing-basics.html and hope that gets me far enough. Let me know if you have any recommendations for learning material.

JanMarvin commented 2 years ago

Hi @David-Rattray , welcome back and I hope you're better now! I'd write a test that expects a certain row height. Though I do not know if it works as expected. Therefore visual testing is required in the first place.

Btw I closed your initial PR to quick. I can push to your PR, but you can't push to this one. Either we reopen your PR or you open a new one.

David-Rattray commented 2 years ago

@JanMarvin Yeah so would the smartest thing for me be to make a new fork from the development branch to get the changes you implemented, then use that to add the feature tests and then create a new pull request when I'm finished? Or would there be a better branch to grab the code changes from?

Edit: Just looked again, probably should clone/fork from auto_resize?

David-Rattray commented 2 years ago

@JanMarvin to save you replying I just cloned auto_resize branch and am working away. Feature tests on hold for the moment. Was looking at the code closer, it appears all the auto does is add the extra height parameter to the base height parameter. Going to look closer at what setColumnWidths does to auto adjust and see if I can implement something similar for row heights.

JanMarvin commented 2 years ago

But wasn't that expected? I assumed that it calculates which row height is required and this value is adjusted in the workbook.

David-Rattray commented 2 years ago

Yeah I jumped the gun a bit when I was reading the functions in wrappers.R didn't realize the autoHeights bit was under helper functions. Seemed like it was just adding a static extra height parameter not a calculated one and the test output has a slightly too short row height for the sample text.