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

Allow the creation of R Clients from other libraries (useful for DnD and in general) #4441

Merged
merged 11 commits into from
Sep 6, 2023

Conversation

jcferretti
Copy link
Member

@jcferretti jcferretti commented Sep 5, 2023

I ran the testthat tests, they are passing.

@jcferretti jcferretti added this to the August 2023 milestone Sep 5, 2023
@jcferretti jcferretti self-assigned this Sep 5, 2023
Copy link
Contributor

@alexpeters1208 alexpeters1208 left a comment

Choose a reason for hiding this comment

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

We need, at minimum:

  1. Test case for providing an XPtr to the Client constructor.
    a. Construct a client connection from client1 <- Client$new(target = ...)
    b. Create a simple static table with empty_table and bind to a variable
    c. Extract the external pointer with xptr <- client1$.internal_rcpp_object$.pointer
    d. Construct a client connection from client2 <- Client$new(xptr)
    e. Verify you can pull table with client2$open_table("table")
  2. A test case where the first and only argument to Client$new() is not a string, not a list, and not an external pointer. Error handling as appropriate.
  3. If make_table_handle_from_ticket is for external use, we need an R6-level wrapping, error handling, and (at least) negative tests as appropriate. If it is internal use, I do not see any cases of its usage.

Figure out an informative error when argument 1 to Client$new() is not acceptable, and mention that we will also accept external pointers to existing client connections.

@jcferretti
Copy link
Member Author

Tests passing at this point, including new tests for arguments to initialize and make_table_handle_from_ticket.

> library(testthat)
> test_package("rdeephaven")
Loading required package: Rcpp
Loading required package: arrow

Attaching package: ‘arrow’

The following object is masked from ‘package:testthat’:

    matches

The following object is masked from ‘package:utils’:

    timestamp

Loading required package: R6
Loading required package: dplyr

Attaching package: ‘dplyr’

The following object is masked from ‘package:testthat’:

    matches

The following objects are masked from ‘package:stats’:

    filter, lag

The following objects are masked from ‘package:base’:

    intersect, setdiff, setequal, union

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 302 ]
> 

@jcferretti jcferretti enabled auto-merge (squash) September 6, 2023 04:33
@jcferretti jcferretti merged commit fa4fee9 into deephaven:main Sep 6, 2023
@jcferretti jcferretti deleted the cfs-r-cpp-2 branch September 6, 2023 15:39
@github-actions github-actions bot locked and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants