CHOP-CGTInformatics / REDCapTidieR

Makes it easy to read REDCap Projects into R
https://chop-cgtinformatics.github.io/REDCapTidieR/
Other
32 stars 8 forks source link

[BUG] Test error with dev version of labelled #189

Closed larmarange closed 5 months ago

larmarange commented 5 months ago

Expected Behavior

Labelled version 2.13.0 is about to be submitted to CRAN.

In this new version, a bug has been fixed in set_value_labels(). The function now calls systematically haven::labelled() and report an error when trying to assign not accepted values.

Current Behavior

With the dev version of labelled, some tests of REDCapTidier are failing. cf. https://win-builder.r-project.org/incoming_pretest/labelled_2.13.0_20240405_134651/reverseDependencies/summary.txt

After exploring, it seems to occur here in test-utils.R:

out <- multi_choice_to_labels(
    db_data = db_data_classic,
    db_metadata = db_metadata_classic,
    raw_or_label = "haven"
  ) %>%
    suppressWarnings(classes = c(
      "empty_parse_warning",
      "field_missing_categories",
      "duplicate_labels"
    ))

Trying to understand when it was coming from, it seems related to the variable "radio_dtxt_error" in db_metadata_classic.

Variable db_data$radio_dtxt_error contains only NA and is of class logical. And it has no value labels in metadata. Therefore the identified labels is only NA and of class logical.

If you try to apply value labels to a logical vector, you will get an error from haven because value labels could be applied only to numeric or to character vectors.

In apply_labs_haven(), I see these lines of code:

  if (!is.null(attr(labels_cast, "problems"))) {
    # If there was parsing problem fall back to character
    x <- as.character(x)
    labels_cast <- force_cast(labels, character())
  }

In this specific case, this condition is not triggerred. In this specific case, it would be important to convert x to character or numeric before calling set_value_labels(). (in current CRAN version of labelled, it doesn't produce an error while it should have done).

It seems that changing the code to

  if (!is.null(attr(labels_cast, "problems")) || is.logical(x)) {
    # If there was parsing problem fall back to character
    x <- as.character(x)
    labels_cast <- force_cast(labels, character())
  }

is enough to solve the bug.

Would you like to consider such change? Could you check if all tests pass?

If yes, would you consider to submit a new version on CRAN before I resubmit the dev version of labelled on CRAN?

How to Reproduce the Bug:

Install dev version of labelled and run tests of REDCapTidier.

Failure Logs

If applicable, submit any failure logs or error messages here.

Checklist

Before submitting this issue, please check and verify below that the submission meets the below criteria:

ezraporter commented 5 months ago

Thanks! Looking into this now and we can certainly resubmit to CRAN with a fix prior to your release.

larmarange commented 5 months ago

Thanks

ezraporter commented 5 months ago

Fixed now in the dev version and confirmed the tests pass with the labelled dev version. Working on the CRAN submission now and will update when that's accepted!

larmarange commented 5 months ago

@ezraporter Thanks a lot. let me know when you expect to submit a new version to CRAN. I will have to wait before resubmitting labelled to CRAN.

Regards

ezraporter commented 5 months ago

Will do! Just put the submission in and will let you know when it's approved

ezraporter commented 5 months ago

Sorry for the delay. We got caught by a website we reference in our docs having an outage and resulting in URL checks failing. CRAN is rerunning checks now and if all goes well the patch should be up by tomorrow if not this evening. Will keep you updated.

larmarange commented 4 months ago

No worry 😄

ezraporter commented 4 months ago

All set! https://cran.r-project.org/web/packages/REDCapTidieR/

larmarange commented 4 months ago

Great. Thanks a lot. I'm currently traveling for a conference but will submit a new version of labelled next week