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

Updates for SAS length #21

Merged
merged 12 commits into from
Feb 6, 2021
Merged

Updates for SAS length #21

merged 12 commits into from
Feb 6, 2021

Conversation

elimillera
Copy link

This closes #20.

I've added a SASlength function to set and return the SASlength attribute similar to the other attribute setters. I've updated the documentation, NAMESPACE, and NEWS.md files as well.

Let me know if there are any changes you want to see before sending to CRAN. R CMD CHECK is all green.

> library(SASxport)
> mtcars2 <- mtcars
> mtcars2$mpg <- as.character(mtcars2$mpg)
> SASlength(mtcars2$mpg) <- 20
> mtcars2$am <- as.character(mtcars2$am)
> write.xport(mtcars2, file = "test.xpt")

Copy link
Member

@warnes warnes left a comment

Choose a reason for hiding this comment

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

Add SASlengrh examples to the two man pages and see the other comments.

@elimillera
Copy link
Author

@warnes Thanks for the quick review. Updates have been made.

@elimillera
Copy link
Author

@warnes Any other changes you'd like to see on this?

@warnes
Copy link
Member

warnes commented Dec 29, 2020

Code looks good! Just a couple of minor nits.

I'm going to pull down the code and run some checks now.

-G

@mstackhouse
Copy link

@warnes Any chance this could be merged soon? The length update here would allow CDISC compliant XPT file generation, which is going to be a huge benefit to the pharma industry's use of R.

@warnes warnes merged commit b089340 into r-gregmisc:master Feb 6, 2021
@elimillera
Copy link
Author

@warnes Do you have a time frame for this being pushed up to CRAN?

@DennisSweitzer
Copy link

Good to see these updates; My team just duplicated this before seeing you fixed it.
We also found another limitation. SAS allows integers to be defined with <8 bytes of memory, but SASxport converts all integers into an 8 byte double before putting it in the buffer.
Are there any plans to fix that? I found the code in xport.numeric() that needs modification, but haven't worked with C libraries before.

@mstackhouse
Copy link

@warnes do you intend to move this version to CRAN anytime soon? It would be immensely valuable to have this PR incorporated and available to install via CRAN.

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.

5 participants