-
Notifications
You must be signed in to change notification settings - Fork 21
feat!(pkg-r): new QueryChat API
#109
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
base: main
Are you sure you want to change the base?
Conversation
QueryChat() APIQueryChat API
|
@gadenbuie ideally we'd also address the data source plugin API (which is a bit of a mess), but I'm gonna punt on that for now #110 |
gadenbuie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nice, going to be a great overall improvement to the package!
pkg-r/R/data_source.R
Outdated
| #' @export | ||
| querychat_data_source <- function(x, ...) { | ||
| UseMethod("querychat_data_source") | ||
| create_data_source <- function(x, table_name = NULL, ...) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're renaming this function, I'd rather as_querychat_data_source(). It's verbose, but developer facing, and reflects that it converts from one data source type to another.
If we're going to revamp this flow when we re-evaluate data sources, I'd prefer that we don't rename this function for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I like that, and I also think we should revamp the plugin interface. Since I'm not ready to sign up for that right now, I'd rather we make a small improvement here than nothing at all.
| # The connection will automatically be closed when the app stops, thanks to | ||
| # querychat_init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you comment on the behavior changes you made in this area?
Especially with the new R6 class, I don't think we should be automatically closing this connection -- or at least we definitely shouldn't be doing that when you call the $app() method.
We should probably create an example where the querychat object lives entirely within the session; i.e. we connect to the database in each session and use renderUI() so that it's entirely session-based. We can also show, in that example, how to clean up the connection.
Otherwise, the more common pattern will be to create the querychat instance in the global environment and use it in both the UI and server. In that use case, it doesn't make sense to clean up the data source except at the session end; it could make sense at app end, but that's something you'd probably want to opt into.
Relatedly, it might be good to have an example that uses pool, or at least to verify that pool would work with querychat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I eventually hit the auto_close code in QueryChat$new() and left a comment there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is a great point, thanks. I hadn't thought through this carefully -- I was just porting what was already going on in querychat_init(). See #109 (comment) for some thoughts on this.
Co-authored-by: Garrick Aden-Buie <[email protected]>
Co-authored-by: Garrick Aden-Buie <[email protected]>
8fe0bd1 to
be0ddda
Compare
…ide of active session
|
Thanks for the detailed feedback @gadenbuie. Just FYI, I'm likely going to merge this by EOD Wed to unblock Veerle. |
|
Also, one thing I realized while addressing feedback is that it's not currently possible to access the app object underlying |
This PR introduces a major API redesign for the R package, replacing the functional API with a simpler R6 class-based approach. These changes are in part motivated by the recent changes made to the Python package (#101 and #108), where the class-based approach is much more idiomatic to Python and Shiny Express.
Summary
The previous API required users to juggle multiple function calls (
querychat_init(),querychat_sidebar(),querychat_server()), explicitly create data sources withquerychat_data_source(), and route output/input values into the proper locations. The newQueryChatR6 class consolidates all this functionality into a single object.API Comparison
Before:
After:
Key Changes
New QueryChat R6 Class
QueryChat$new(data_source, table_name, ...)accepts data frames or database connections directly$sidebar(),$ui()create chat interface components$server()initializes server logic (must be called within Shiny server function)$df(),$sql(),$title()provide reactive access to current state$app()generates a ready-to-run Shiny app with sensible defaults$generate_greeting()creates reusable greeting messages$cleanup()closes data source resourcesHard Deprecations
All previous functional API functions now throw errors with helpful migration messages:
querychat_init()→QueryChat$new()querychat_app()→QueryChat$app()querychat_sidebar()→QueryChat$sidebar()querychat_ui()→QueryChat$ui()querychat_server()→QueryChat$server()querychat_greeting()→QueryChat$generate_greeting()querychat_data_source()→QueryChat$new()(data sources now created internally)Internal Refactoring
querychat_data_source()tocreate_data_source()- now exported as an internal developer extension point onlycategorical_thresholdfrom data source creation (moved toQueryChat$new())Migration Guide
Users upgrading from the old API will receive clear error messages with side-by-side code comparisons showing exactly how to migrate. The new API is more intuitive and requires less code in most cases.
Documentation Updates