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

Stop advertising functions that don't fit in openxlsx2 API #694

Merged
merged 13 commits into from
Jul 18, 2023

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Jul 17, 2023

This PR aims to modify the function index https://janmarvin.github.io/openxlsx2/dev/reference/index.html, so that functions that don't fit in the openxlsx2 API are not advertised.

This will not create breakages, as all functions are still exported.

Adds a bunch of @keywords internal to the functions that don't fit in the API. (functions still exported, but not advertised)

Relocate wb_dims on top of rowcol_to_dims() dims_to_rowcols(), so it shows up first.

This PR is minimal. and a overhaul of examples, descriptions links, and deprecation warnings can be done elsewhere (wanted to keep it small)

Edit: this PR got a little larger, because wb_add_data() and write_data() were documented together. I separated the docs, to make sure that wb_add_data() was not marked with @keywords internal

Edit2 : I was not able to reproduce interactively, but when running R CMD Check in RStudio's build pane. I get the following error.

   Running the tests in 'tests/testthat.R' failed.
   Last 13 lines of output:
     Error in `basename(sheets$Target)`: object 'sheets' not found
     Backtrace:1. └─openxlsx2::wb_load(xlsxFile) at test-write.R:135:2
      2.   └─base::basename(sheets$Target)

@olivroy olivroy changed the title Stop advertising "outlier" functions Stop advertising functions that don't fit in openxlsx2 API Jul 17, 2023
…to make sure it shows up on function reference.
Copy link
Owner

@JanMarvin JanMarvin left a comment

Choose a reason for hiding this comment

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

And the R CMD check should be investigated too

R/utils.R Outdated Show resolved Hide resolved
R/class-workbook-wrappers.R Outdated Show resolved Hide resolved
@JanMarvin
Copy link
Owner

Thanks! since we have two interfaces, the lack of exports is something I'm constantly in fear of.

Do you still see the r cmd check, I haven't looked into this yet?

@olivroy
Copy link
Collaborator Author

olivroy commented Jul 18, 2023

HI @JanMarvin,

regarding the R CMD check, I don't really what is going on. It is failing because of tests only (i.e. only 1 ERROR) It is probably related to a setup on my computer, Windows, R 4.2.3, RStudio 2023.06.0

  • This PR basically does nothing.
  • It passes when I use Ctrl + Shift + T
  • it passes when I run tests interactively. devtools::test()
  • It just doesn't pass with RStudio Check command Ctrl + Shift + E
  • I see the same thing on main on my machine.

Here is a snippet of the back trace I get.

── Warning ('test-class-comment.R:62:3'): load comments ────────────────────────
internal error in 'unz' code
Backtrace:1. └─openxlsx2::wb_load(fl) at test-class-comment.R:62:2
 2.   └─utils::unzip(file, exdir = xmlDir)
── Warning ('test-class-workbook-wrappers.R:222:3'): wb_add_drawing is a wrapper ──
internal error in 'unz' code
Backtrace:1. └─openxlsx2::wb_load(file = fl) at test-class-workbook-wrappers.R:222:2
 2.   └─utils::unzip(file, exdir = xmlDir)
── Warning ('test-class-workbook.R:328:3'): clone worksheet ────────────────────
internal error in 'unz' code
Backtrace:1. └─openxlsx2::wb_load(fl) at test-class-workbook.R:328:2
 2.   └─utils::unzip(file, exdir = xmlDir)
── Warning ('test-class-workbook.R:737:3'): changing sheet names works with named regions ──
zip file is corrupt
Backtrace:1. └─openxlsx2::wb_load(filename) at test-class-workbook.R:737:2
 2.   └─utils::unzip(file, exdir = xmlDir)
── Warning ('test-cloneWorksheet.R:9:3'): clone Worksheet with data ────────────
zip file is corrupt

I checked on R-devel, nothing seems related.

Only thing that was fixed in R 4.2.2

Files can now be extracted even from very large zip archives ([PR#18390](https://bugs.r-project.org/show_bug.cgi?id=18390), thanks to Martin Jakt).

Edit: other examples..

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-class-comment.R:62:3'): load comments ──────────────────────────
Error in `basename(sheets$Target)`: object 'sheets' not found
Backtrace:1. └─openxlsx2::wb_load(fl) at test-class-comment.R:62:2
 2.   └─base::basename(sheets$Target)
── Error ('test-class-workbook-wrappers.R:222:3'): wb_add_drawing is a wrapper ──
Error in `basename(sheets$Target)`: object 'sheets' not found
Backtrace:1. └─openxlsx2::wb_load(file = fl) at test-class-workbook-wrappers.R:222:2
 2.   └─base::basename(sheets$Target)
── Error ('test-class-workbook.R:328:3'): clone worksheet ──────────────────────
Error in `basename(sheets$Target)`: object 'sheets' not found
Backtrace:1. └─openxlsx2::wb_load(fl) at test-class-workbook.R:328:2
 2.   └─base::basename(sheets$Target)
── Error ('test-class-workbook.R:737:3'): changing sheet names works with named regions ──
Error in `basename(sheets$Target)`: object 'sheets' not found
Backtrace:1. └─openxlsx2::wb_load(filename) at test-class-workbook.R:737:2
 2.   └─base::basename(sheets$Target)
── Error ('test-cloneWorksheet.R:9:3'): clone Worksheet with data ──────────────
Error in `basename(sheets$Target)`: object 'sheets' not found
Backtrace:1. └─openxlsx2::wb_load(file = file_name) at test-cloneWorksheet.R:9:2
 2.   └─base::basename(sheets$Target)
── Error ('test-cloneWorksheet.R:23:3'): clone empty Worksheet

@JanMarvin
Copy link
Owner

Hm, maybe a download didn't succeed? There isn't really a to big file in our tests. You could clone the git repo.

@olivroy
Copy link
Collaborator Author

olivroy commented Jul 18, 2023

Maybe a couple of skip_on_cran() could fix that? but if you never had these problems before, I think it is safe to assume it is related to my machine, and we can ignore until new information convinces us to do otherwise

@JanMarvin
Copy link
Owner

All good on my end

[ FAIL 0 | WARN 0 | SKIP 1 | PASS 1375 ]

@JanMarvin JanMarvin merged commit 48bf19f into JanMarvin:main Jul 18, 2023
@JanMarvin
Copy link
Owner

Thanks, merged!

@olivroy
Copy link
Collaborator Author

olivroy commented Jul 18, 2023

Great I am glad it passes on your end.. I really have no idea.. If I figure it out, I will let you know.

I found this link, but haven't looked at it yet! https://community.rstudio.com/t/devtools-test-passes-but-devtools-check-does-not/129885/7

@olivroy olivroy deleted the fn-index-df branch July 18, 2023 17:24
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.

2 participants