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

Fix non-chr/numeric casting in apply_labs_haven() #190

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

ezraporter
Copy link
Collaborator

@ezraporter ezraporter commented Apr 10, 2024

Description

This PR fixes test failures occurring against the development version of labelled().

My initial implementation of apply_labs_haven() was more general than it needed to be and introduced a test failure on a radio button variable where both the data values and labels are missing and therefore NA_logical. haven::labelled() explicitly accepts only character and numeric data types so we don't actually have to account for data values of other data types and can just cast to character when they occur.

I also removed a failing test for a specific error message when the API returns status code 405. We were hitting google to generate the status code and it now returns 404. Not sure it's worth testing these.

Proposed Changes

  • Remove failing status 405 test case
  • Update force_cast() and apply_labs_haven() to fall back to converting to character when non-numeric data types are encountered

Issue Addressed

closes #189

PR Checklist

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

  • New/revised functions have associated tests
  • New/revised functions that update downstream outputs have associated static testing files (.RDS) updated under inst/testdata/create_test_data.R
  • New/revised functions use appropriate naming conventions
  • New/revised functions don't repeat code
  • Code changes are less than 250 lines total
  • Issues linked to the PR using GitHub's list of keywords
  • The appropriate reviewer is assigned to the PR
  • The appropriate developers are assigned to the PR
  • Pre-release package version incremented using usethis::use_version()

Code Review

This section to be used by the reviewer and developers during Code Review after PR submission

Code Review Checklist

  • I checked that new files follow naming conventions and are in the right place
  • I checked that documentation is complete, clear, and without typos
  • I added/edited comments to explain "why" not "how"
  • I checked that all new variable and function names follow naming conventions
  • I checked that new tests have been written for key business logic and/or bugs that this PR fixes
  • I checked that new tests address important edge cases

@ezraporter ezraporter added the bug Something isn't working label Apr 10, 2024
@ezraporter ezraporter requested a review from rsh52 April 10, 2024 16:15
@ezraporter ezraporter self-assigned this Apr 10, 2024
Copy link
Collaborator

@rsh52 rsh52 left a comment

Choose a reason for hiding this comment

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

Two small comments, otherwise looks good. 👍

NEWS.md Outdated Show resolved Hide resolved
tests/testthat/test-read_redcap.R Show resolved Hide resolved
Co-authored-by: Rich Hanna <[email protected]>
@ezraporter ezraporter merged commit dbb14d8 into main Apr 10, 2024
4 checks passed
@ezraporter ezraporter deleted the patch-test-failures branch April 10, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Test error with dev version of labelled
2 participants