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(r): Ensure driver, database, connection, and statement classes work with S4 slots #876

Closed
wants to merge 4 commits into from

Conversation

paleolimbot
Copy link
Member

@paleolimbot paleolimbot commented Jul 6, 2023

For frameworks that use S4 (notably, DBI), slot type enforcement is useful when the underlying object is an external pointer. This PR adds setOldClass() calls such that you can use adbc_driver/adbc_database/adbc_connection/adbc_statement as slots.

I also noticed a copy/paste error for the driver class names...this PR also updates them to be more consistent: after this PR, drivers are always pkgname_driver; databases are always pkgname_database, and so on.

Before this PR:

library(adbcdrivermanager)

setClass("SomeClass",
  slots = list(
    driver = "adbc_driver",
    database = "adbc_database",
    connection = "adbc_connection",
    statement = "adbc_statement"
  )
)
#> Warning: undefined slot classes in definition of "SomeClass": driver(class
#> "adbc_driver"), database(class "adbc_database"), connection(class
#> "adbc_connection"), statement(class "adbc_statement")

setMethod("initialize", "SomeClass", function(.Object, driver, ...) {
  .Object@driver <- driver
  .Object@database <- adbc_database_init(.Object@driver, ...)
  .Object@connection <- adbc_connection_init(.Object@database)
  .Object@statement <- adbc_statement_init(.Object@connection)
  .Object
})


new("SomeClass", adbc_driver_void())
#> Error in (function (cl, name, valueClass) : c("assignment of an object of class \"adbc_driver_void\" is not valid for @'driver' in an object of class \"SomeClass\"; is(value, \"adbc_driver\") is not TRUE", "assignment of an object of class \"adbc_driver\" is not valid for @'driver' in an object of class \"SomeClass\"; is(value, \"adbc_driver\") is not TRUE", "assignment of an object of class \"adbc_xptr\" is not valid for @'driver' in an object of class \"SomeClass\"; is(value, \"adbc_driver\") is not TRUE")

Created on 2023-07-06 with reprex v2.0.2

After this PR:

library(adbcdrivermanager)

setClass("SomeClass",
  slots = list(
    driver = "adbc_driver",
    database = "adbc_database",
    connection = "adbc_connection",
    statement = "adbc_statement"
  )
)

setMethod("initialize", "SomeClass", function(.Object, driver, ...) {
  .Object@driver <- driver
  .Object@database <- adbc_database_init(.Object@driver, ...)
  .Object@connection <- adbc_connection_init(.Object@database)
  .Object@statement <- adbc_statement_init(.Object@connection)
  .Object
})


new("SomeClass", adbc_driver_void())
#> An object of class "SomeClass"
#> Slot "driver":
#> <adbc_driver_void> List of 1
#>  $ driver_init_func:Class 'adbc_driver_init_func' <externalptr> 
#> 
#> Slot "database":
#> <adbc_database> <pointer: 0x14bf4c450> List of 2
#>  $ driver :<adbc_driver_void> List of 1
#>   ..$ driver_init_func:Class 'adbc_driver_init_func' <externalptr> 
#>  $ options: list()
#> 
#> Slot "connection":
#> <adbc_connection> <pointer: 0x14bfbbf60> List of 2
#>  $ database:<adbc_database> <pointer: 0x14bf4c450> List of 2
#>   ..$ driver :<adbc_driver_void> List of 1
#>   .. ..$ driver_init_func:Class 'adbc_driver_init_func' <externalptr> 
#>   ..$ options: list()
#>  $ options : list()
#> 
#> Slot "statement":
#> <adbc_statement> <pointer: 0x14bf93700> List of 2
#>  $ connection:<adbc_connection> <pointer: 0x14bfbbf60> List of 2
#>   ..$ database:<adbc_database> <pointer: 0x14bf4c450> List of 2
#>   .. ..$ driver :<adbc_driver_void> List of 1
#>   .. .. ..$ driver_init_func:Class 'adbc_driver_init_func' <externalptr> 
#>   .. ..$ options: list()
#>   ..$ options : list()
#>  $ options   : list()

Created on 2023-07-06 with reprex v2.0.2

@paleolimbot paleolimbot marked this pull request as ready for review July 6, 2023 17:29
@paleolimbot
Copy link
Member Author

@krlmlr and/or @nbenn can you have a look at this to see if it will satisfy the requirements of a future DBI wrapper?

@krlmlr
Copy link
Collaborator

krlmlr commented Jul 18, 2023

Thanks. Should we discuss this on Thursday?

@paleolimbot
Copy link
Member Author

Happy to discuss synchronously or asynchronously! If this is at all useful I'm happy to include it...if it's not at all useful I'm happy to close.

If you're using the built-in type enforcement on S4 slots, it probably makes sense for drivers to declare setOldClass() (or else the adbi package will have to be aware of every driver/database/connection/statement subclass that exists?). If setting the slot type as ANY is OK, I imagine the setOldClass() calls (in either package) are not all that useful?

@paleolimbot
Copy link
Member Author

Closing in favour of not using S4-style type checking in the DBI wrapper!

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.

2 participants