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

port write_dwc() to use API #297

Closed
16 tasks done
PietrH opened this issue Sep 12, 2024 · 2 comments · Fixed by #302
Closed
16 tasks done

port write_dwc() to use API #297

PietrH opened this issue Sep 12, 2024 · 2 comments · Fixed by #302
Assignees
Labels
actionable Can be implemented API enhancement New feature or request
Milestone

Comments

@PietrH
Copy link
Member

PietrH commented Sep 12, 2024

Base on vcr branch and immediately implement cached API responses for tests

  • deprecate connection
  • split tests: local connection
  • split tests: api
  • check_opencpu_response() should check for content type of API response #300
  • cache API response
  • implement sql helper
  • test sql helper
  • update NEWS?
  • deprecate directory
  • reinstate directory
  • ignore passing directory to helper functions: add ignore_arguments argument to conduct_parent_to_helpers()
  • setup optional writing functionality after conduct_parent_to_helpers()
  • make sure write_dwc(api = TRUE) uses RDS/compression
  • Check if I can use DBI::dbDisconnect() after all, it seems to work for the other functions, so why not here?
  • move tests to single file
  • R CMD CHECK fails on v2.3 #307
@PietrH
Copy link
Member Author

PietrH commented Sep 12, 2024

write_dwc() is the only function that has a deprecated argument other than connection: directory, this argument is passed in the payload and causing an invalid request to openCPU.

This is because the arguments that should not be passed to the API are hardcoded in the conduct_parent_to_helpers() function:

etn/R/utils.R

Lines 354 to 362 in ee49b2d

# Get the argument values from the parent function
arguments_to_pass <-
return_parent_arguments(depth = 2)[
!names(return_parent_arguments(depth = 2)) %in% c(
"api",
"connection",
"function_identity"
)
]

It might be better if deprecated arguments could be explicitly passed to this function as arguments, the api argument should never be passed to etnservice

@PietrH PietrH added this to the v3.0.0 milestone Sep 12, 2024
@PietrH PietrH linked a pull request Sep 13, 2024 that will close this issue
@PietrH
Copy link
Member Author

PietrH commented Sep 16, 2024

I can use DBI::dbDisconnect() here, and everywhere, because a connection is created for every query, and then closed again after the query.

In etnservice we had a connection leak causing too much resources to be used and the database to run out of connections. However, there might be a performance penalty.

I'm leaving it in for now.

@PietrH PietrH closed this as completed Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionable Can be implemented API enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant