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

Add new core function xportr_varnames() #11

Merged
merged 13 commits into from
Jan 6, 2022

Conversation

AARON-CLARK
Copy link

@AARON-CLARK AARON-CLARK commented Nov 16, 2021

Greetings!

Sorry for the delay in my proposed feature add, but this PR helps achieve #1. Essentially, it proposes xportr_varnames as a new potential core function to:

(1) make sure .df passes all the required variable name checks, and if it doesn't...
(2) automatically rename the colnames of .df into compliance

If you don't think adding another core function is a good approach, I also added an new argument to xportr_write which called tidy_varnames which accomplishes the same task.

I tried my best to update all the documentation via the .Rd files so that developers can understand each function's purpose & process. I augmented the main "Getting Started" vignette (xportr/docs/articles/xportr.html) with a whole new section to showcase my vision for the function's use at a high level, so I'd start there when reviewing the code.

I held off on having these new functions interface with the spec / metacore object until I got some feedback from your team. Obviously, if a user is changing a variable name in .df, you'd need it to match a corresponding variable in the spec/ metacore object. However, it felt... risky... changing the spec file on the fly given that's a document/ dataset that needs to be highly reviewed by peers. I did add an alternative way the user can alter the spec / metacore object directly using a second exported function called xportr_tidy_rename that could be used in a dplyr::mutate() and is highlighted in xportr/docs/articles/xportr.html as well. Anyway, I'd like to hear your take and would be happy to make any changes.

As a reminder, these are the variable name requirements I designed for: variable names must ...

  • be a maximumof 8 characters
  • ASCII characters only
  • start with a letter (not an underscore)
  • contain only uppercase letters & numbers, meaning no other symbols or special characters including underscores. However, legacy studies started on or before December 17, 2016, may use the underscore character "_" according to the FDA's conformance guide, section 3.3.6.

Looking forward to your feedback! I know it's a lot to ingest. Feel free to ask questions as they come up.

@bms63
Copy link
Collaborator

bms63 commented Nov 19, 2021

Wow! Thanks so much @AARON-CLARK! We will be looking at this soon. @elimillera

@AARON-CLARK AARON-CLARK changed the title Add new core function xportr_varnames() Add new core function xportr_varnames() Nov 19, 2021
Copy link
Member

@elimillera elimillera left a comment

Choose a reason for hiding this comment

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

Thanks @AARON-CLARK, This looks really useful. I think we just need to add tm to the DESCRIPTION and the build should work here.

DESCRIPTION Outdated Show resolved Hide resolved
@AARON-CLARK
Copy link
Author

@elimillera or @bms63, just circling back to this PR after the new year. Since I am an out-sider, I can't run any workflows. Could one of you re-run? Thanks!

image

@bms63 bms63 merged commit fe8c7cd into atorus-research:master Jan 6, 2022
@bms63
Copy link
Collaborator

bms63 commented Jan 6, 2022

Hey Aaron,

Many thanks for working on this! Things are falling over at the moment with the tests, but I will try and investigate ASAP. @elimillera not sure if you are back from vacay, but if you have time maybe we could tag team this.

In other news, it looks like xportr is being adopted into something called the pharmaverse, so we are going to be actively working on this package in the very near future.

xportr will hopefully work very nicely with this https://roche-gsk.github.io/admiral/index.html package (ADaM in R Asset library) and the metacore package (https://atorus-research.github.io/metacore/ in the hopes of creating an E2E pipeline for doing Clinical Reporting in R!

@AARON-CLARK
Copy link
Author

Hi @bms63,

Yes, I'm very familiar with admiral & metacore in the ever expanding pharmaverse. Its truly an exciting time to be an R developer in the Pharma industry. But thanks for reviewing my PR - I'm happy to contribute a small piece of the puzzle.

Speaking of tests, I need to follow up with another PR. I was delaying writing testthat tests for each utility function that supports xportr_tidy_rename() and xportr_varnames() in #11 until I had some feedback. But since it's merged, I'll go ahead and write them as soon as I can.

Thanks!

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.

3 participants