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

RS-3782 save load to datamart blob #1

Merged
merged 10 commits into from Oct 4, 2019

Conversation

jeremytay90
Copy link
Contributor

There are 2 warnings showing up when checking, but both relate to close.qpostconn parameters (documentation/consistency) but I don't think it's important?

@jeremytay90 jeremytay90 requested review from mwmclean and removed request for mwmclean September 27, 2019 07:18
R/DataMart.R Outdated

res <- try(POST(paste0(url, "?filename=", filename),
companySecret <- ifelse(exists("companySecret"), companySecret, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

ifelse() is not great for a few reasons. you can replace this with companySecret <- get0("companySecret", ifnotfound = "")

@jeremytay90 jeremytay90 changed the title Rs 3782 save load to datamart blob RS-3782 save load to datamart blob Oct 1, 2019
Copy link
Contributor

@mwmclean mwmclean left a comment

Choose a reason for hiding this comment

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

close.qpostconn and QSaveData should end with calls to invisible() and their @return should say they are called for their side effect and return NULL invisibly.

It also might be nice if the error message for those functions when the POST call fails, included more information such as the http error code.

@jeremytay90
Copy link
Contributor Author

close.qpostconn and QSaveData should end with calls to invisible() and their @return should say they are called for their side effect and return NULL invisibly.

It also might be nice if the error message for those functions when the POST call fails, included more information such as the http error code.

I've added the http status code, but since there is no response content and the error message is in the status text, I think this is the best I can do ...

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.8%) to 20.784% when pulling 22c5547 on RS-3782-SaveLoadToDatamartBlob into 7fd6a22 on master.

@mwmclean mwmclean merged commit d7acd1b into master Oct 4, 2019
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.

None yet

3 participants