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

feat: format errors #21

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

timcadman
Copy link

Implement two main changes:

  1. Added argument return_error to datashield.assign functions, giving the option either to immediately return errors or to store them to be accessible with a call to datashield.errors
  2. Added styling to the output returned by datashield.errors.

Note: tests written for datashield.assign.expr; however I was unable to create errors with either datashield.assign.table or datashield.assign.resource. If you have code which will do this tell me and I'll add tests.

Copy link
Member

@ymarcon ymarcon left a comment

Choose a reason for hiding this comment

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

Not tested yet, please fix dependencies and documentation. Also I expect the man folder to be updated. In case you do not know how to do it in Rstudio, make sure in the project options that all the boxes are checked:

image

so that documentation is generated at check time.

R/datashield.assign.R Show resolved Hide resolved
R/datashield.assign.R Outdated Show resolved Hide resolved
.checkLastErrors()
if(return_errors == TRUE){
cli_text()
cli_alert_warning("Error: There are some DataSHIELD errors ")
Copy link
Member

Choose a reason for hiding this comment

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

What happens when the execution is not in a CLI context, like a batch script execution?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean eg calling datashield.assign.expr in a loop? Or you had something else in mind?

Copy link
Member

Choose a reason for hiding this comment

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

No, I mean when there is no command line, like when executing code with Rscript.

@timcadman
Copy link
Author

Not tested yet, please fix dependencies and documentation. Also I expect the man folder to be updated. In case you do not know how to do it in Rstudio, make sure in the project options that all the boxes are checked:

image

so that documentation is generated at check time.

Sorry Yannick, not sure what happened yesterday ... trying to do 17 things at once!

@timcadman
Copy link
Author

Hi Yannick, following your comments:

  • I've now tested this with an external script (tests added to package).
  • You were right to suggest this, as it highlighted an issue as currently I was returning warnings, not errors. I've tweaked further now so an error is returned and this gives the correct behaviour (throwing error when running a script).

Copy link
Member

@ymarcon ymarcon left a comment

Choose a reason for hiding this comment

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

Please rebase to master, there are conflicts in the NAMESPACE.

Copy link
Member

Choose a reason for hiding this comment

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

Please add .DS_Store to .gitignore and .Rbuildignore (thank you Apple for the noise).

#' @importFrom purrr map
#' @importFrom stringr str_replace_all
.remove_curly_brackets <- function(errors) {
errors <- errors %>% map(~str_replace_all(.x, "\\{", "("))
Copy link
Member

Choose a reason for hiding this comment

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

Try to minimize the package dependencies. I don't think you need all the fancy packages dplyr, purr and stringr for that function, just go the plain old R way:

# Replace all curly brackets with parentheses
output_string <- gsub("\\{", "(", input_string)
output_string <- gsub("\\}", ")", output_string)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants