report icon indicating copy to clipboard operation
report copied to clipboard

CRAN update 22/08

Open DominiqueMakowski opened this issue 2 years ago • 3 comments

Please see the problems shown on https://cran.r-project.org/web/checks/check_results_report.html.

Please correct before 2022-08-22 to safely retain your package on CRAN.

DominiqueMakowski avatar Aug 09 '22 01:08 DominiqueMakowski

The relevant error is:

Error in data_findcols(table, candidates) :
  could not find function "data_findcols"
Calls: <Anonymoous> ... report_statistics -> report_statistics.lm -> .find_regression_estimate
Execution halted

I started getting the same issue in my GitHub actions because I have a vignette relying on report. I could reproduce locally after uninstalling the GitHub version of report and installing the CRAN version. Thus the error seems to be due to the migration of data_findcols and friends to datawizard. A hotfix would probably just update the CRAN version with those updates.

Furthermore, in the CRAN version of report, the DESCRIPTION file imports datawizard (>= 0.2.3), whereas it should probably be 0.4.1 to account for these changes.

rempsyc avatar Aug 10 '22 00:08 rempsyc

I had a look at the CRAN version 0.5.1 and here is the line to correct in file report.lm (line 557):

https://github.com/cran/report/blob/master/R/report.lm.R#L550

#' @keywords internal
.find_regression_estimate <- function(table, ...) {
  candidates <- c("^Coefficient", "beta", "Median", "Mean", "MAP")
  coefname <- attributes(table)$coefficient_name
  if (!is.null(coefname) && coefname %in% names(table)) {
    estimate <- attributes(table)$coefficient_name
  } else {
    estimate <- data_findcols(table, candidates)[1]
  }
  estimate
}

So replace data_findcols with datawizard::data_find

I would do a PR but I don't know how to do that for a CRAN package since it's a different repo.

Edit: Actually, there's a lot more (including data_remove calls, etc.) in many of the functions so I won't list them all here.

rempsyc avatar Aug 10 '22 00:08 rempsyc

Thanks, @rempsyc, for your willingness to help. These issues have actually been already fixed in the GitHub version.

It's just a matter of submitting the package to CRAN :)

IndrajeetPatil avatar Aug 10 '22 05:08 IndrajeetPatil

Where is format_text() gone, or has it always been in datawizard and not in report? We have failing tests, because format_text() now inserts new lines where no new line shouldn't be. format_text() is no in report, only in datawizard, however, it seems that the implementation in datawizard always inserted newlines since the beginning. I wonder whether we had an internal function in report before? I'm not sure what format_text() was supposed to do, and the datawizard implementation does not work well:

Expected result, according to tests:

"The model's intercept, corresponding to f = 1, is at 0.17 (95% CI [-0.47, 0.81], t(27) = 0.55, p = 0.584)."

Actual result:

"The model's intercept, corresponding to f = 1"
", is at 0.17 (95% CI [-0.47, 0.81], t(27) = 0.55, p = 0.584)."

Note the strange linebreak, with the second line starting with a comma.

strengejacke avatar Aug 18 '22 06:08 strengejacke

format_text() moved from report to datawizard about 15 months ago. Not sure what change caused the regression in the datawizard version.

See if you can spot anything in git blame:

https://github.com/easystats/datawizard/blame/33ebba36bc5480434f6f7eb5bd55046842542570/R/format_text.R

IndrajeetPatil avatar Aug 18 '22 06:08 IndrajeetPatil

I think it must be this commit: https://github.com/easystats/datawizard/commit/e868f3b403abdfe3c2d5926c0fe9b7ec93a939bd

Formerly, format text actually did nothing because width was NULL by default. This check (if-statement) was removed, and now it inserts line breaks.

I think in every occurrence of format_text() where width is not specified, we can simply remove it

strengejacke avatar Aug 18 '22 07:08 strengejacke

Dang it!

That commit was supposed to fix this issue: https://github.com/easystats/report/issues/160

I think in every occurrence of format_text() where width is not specified, we can simply remove it

Sounds good.

IndrajeetPatil avatar Aug 18 '22 07:08 IndrajeetPatil

@DominiqueMakowski I think most important issues are resolved. It's just snapshots that fail, so maybe this is ready for submission.

strengejacke avatar Aug 18 '22 11:08 strengejacke

Any update on this?

We are 2 days away from getting archived.

IndrajeetPatil avatar Aug 20 '22 11:08 IndrajeetPatil

How do we resolve the snapshot errors? https://github.com/easystats/report/runs/7924218874?check_suite_focus=true

Let's see if all tests pass, and then we should be ok to submit.

strengejacke avatar Aug 20 '22 12:08 strengejacke

Snapshot tests won't be run on CRAN machines, so this shouldn't interfere with CRAN submission.

I don't have R-devel installed on my Windows machine, so can't update snapshots for Windows. I think we should be skipping those tests on devel. The difference is not concerning:

  - "  R2 = 0.67). The model's intercept, corresponding to cyl = 0, is at 2.94 (95% CI"
  + "  R2 = 0.67). The model's intercept, corresponding to cyl = 0, is at 2.93 (95% CI"

IndrajeetPatil avatar Aug 20 '22 12:08 IndrajeetPatil

All tests pass now.

IndrajeetPatil avatar Aug 20 '22 14:08 IndrajeetPatil

Will submit then, thanks!!

DominiqueMakowski avatar Aug 21 '22 00:08 DominiqueMakowski

thanks, package report_0.5.5.tar.gz is on its way to CRAN.

thanks!

DominiqueMakowski avatar Aug 22 '22 09:08 DominiqueMakowski