Closed rsh52 closed 1 year ago
I think a warning is probably the way to go here. If no extension is supplied, should we automatically add ".xlsx"?
Get Outlook for iOShttps://aka.ms/o0ukef
From: Rich Hanna @.> Sent: Wednesday, May 31, 2023 10:28:22 PM To: CHOP-CGTInformatics/REDCapTidieR @.> Cc: Stephan Kadauke @.>; Mention @.> Subject: [External]Re: [CHOP-CGTInformatics/REDCapTidieR] 0.4 Small Updates (PR #155)
@rsh52 commented on this pull request.
In utility/cli_message_examples_reprex.mdhttps://github.com/CHOP-CGTInformatics/REDCapTidieR/pull/155#discussion_r1212274756:
+#> Warning: Empty string detected for a given multiple choice label. +#> Error in
write_redcap_xlsx()
: +#> ✖add_labelled_column_headers
declared TRUE, but no variable labels +#> detected. +#> ℹ Did you runmake_labelled()
on the supertibble? + +read_redcap(redcap_uri, classic_token) %>%
- write_redcap_xlsx(file = "temp.pdf") +#> Warning in read_redcap(redcap_uri, classic_token): ! Multiple values are mapped to the label
A
in fieldradio_duplicate_label
+#> ℹ Consider making the labels forradio_duplicate_label
unique in your REDCap +#> project +#> Warning: ! The field radio_dtxt_error in data_field_types is a radio field type, however +#> it does not have any categories. +#> Warning: Empty string detected for a given multiple choice label. +#> Error inwrite_redcap_xlsx()
: +#> ✖ Invalid file extension supplied tofile
: pdf
Using wb$save() will execute for pretty much any extension, but the files themselves will be corrupted, produce warnings, or be not be openable. In testing, CSV and XLS save but they open with warnings. docx and pdf will save, but the files won't open. When I tested no file extension it was an empty file.
I guess it's a determination of if we want to fail loudly or silently and let users discover for themselves if their files work. CSV and XLS will be openable, but users will have to convert the files to XLSX to retain the XLSX features we have in our output.
— Reply to this email directly, view it on GitHubhttps://github.com/CHOP-CGTInformatics/REDCapTidieR/pull/155#discussion_r1212274756, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACTGHWUS4JNN6ZXUFQB6QTTXI6SWNANCNFSM6AAAAAAYUOJE4M. You are receiving this because you were mentioned.Message ID: @.***>
This email originated from an EXTERNAL sender to CHOP. Proceed with caution when replying, opening attachments, or clicking links. Do not disclose your CHOP credentials, employee information, or protected health information to a potential hacker.
I think a warning is probably the way to go here. If no extension is supplied, should we automatically add ".xlsx"?
Sounds good. Changed to warning, slight update to message. Up to you, FWIW this isn't behavior seen in base R write.*
or readr
write_*
. For each of those a nondescript file gets made unless you specify an extension, despite the name of the function, but we've typically broken the mold with this package anyway.
I think a warning is probably the way to go here. If no extension is supplied, should we automatically add ".xlsx"?
Sounds good. Changed to warning, slight update to message. Up to you, FWIW this isn't behavior seen in base R
write.*
orreadr
write_*
. For each of those a nondescript file gets made unless you specify an extension, despite the name of the function, but we've typically broken the mold with this package anyway.
Yes you're right. Just one place where we're doing a better job than posit :)
I think a warning is probably the way to go here. If no extension is supplied, should we automatically add ".xlsx"?
Sounds good. Changed to warning, slight update to message. Up to you, FWIW this isn't behavior seen in base R
write.*
orreadr
write_*
. For each of those a nondescript file gets made unless you specify an extension, despite the name of the function, but we've typically broken the mold with this package anyway.Yes you're right. Just one place where we're doing a better job than posit :)
Cool 👍 Solution implemented, corresponding warning messages updated using ChatGPT ✨
Description
This PR builds on top of #154 and addresses some behavior caught by @skadauke in Teams.
Proposed Changes
List changes below in bullet format:
suprtbl_recode()
using label capture and re-establishment similar to what's used inmake_labelled()
to re-establish labels after loss due to data reformattingcheck_file_exists()
(name could be updated, this is also acheckmate
function name) which checks if argoverwrite
isFALSE
inwrite_redcap_xlsx()
and argfile
already exists. Provides a helpful reminder to changeoverwrite
.lifecycle
deprecated functionsAdditional Changes
cli_message_examples.R
with new warnings and checks, rerunreprex
release_questions()
function added that gets called when runningdevtools::release()
. See image below for example output once all standardrelease()
questions have been answered:Issue Addressed
NA
PR Checklist
Before submitting this PR, please check and verify below that the submission meets the below criteria:
.RDS
) updated underinst/testdata/create_test_data.R
httptest::with_mock_api
and any new mocks were added totests/testthat/fixtures/create_httptest_mocks.R
usethis::use_dev_version()
Code Review
This section to be used by the reviewer and developers during Code Review after PR submission
Code Review Checklist