-
Notifications
You must be signed in to change notification settings - Fork 22
fix(pkg-r): querychat_app() should only close connections if it creates the DB
#164
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
pkg-r/R/QueryChat.R
Outdated
| if (is.na(cleanup) && interactive() && !in_shiny_session()) { | ||
| if ( | ||
| is.data.frame(data_source) || | ||
| inherits(data_source, "DataFrameSource") | ||
| ) { | ||
| cleanup <- TRUE | ||
| } | ||
| } |
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.
Should we do the analogous thing for Python?
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.
Yeah
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.
It turns out Python didn't have a cleanup parameter, so I added that to QueryChat.
Python doesn't have a querychat_app() function, which highlights that this is a special case. I also updated the implementation slightly: we now only clean up if we're given a data.frame that we are turning into a data source. Otherwise, calling $cleanup() destructively ruins a DataSource object, which isn't great.
Prior to this PR, if you pass in a database connection to
querychat_app()it will close your connection for you. You only would want that if querychat created the table connection internally, i.e. if you passed in a data.frame.Before
After
The database connection remains intact and the
dbGetQuery()call completes as expected.