-
Notifications
You must be signed in to change notification settings - Fork 20
feat(pkg-py): Add new QueryChat() API; hard deprecate old API
#101
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() API
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
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.
I started reviewing but have talked myself into thinking we should do a clean break API change rather than try to maintain the older functions in a deprecated state. (See #101 (comment))
| "QueryChat", | ||
| "express", |
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 include express in __all__? I kind of think it'd be better to require explicitly reaching into querychat.express.
| "QueryChat", | |
| "express", | |
| "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.
By including express here, it makes this possible:
import querychat
querychat.express.QueryChator, somewhat equivalently:
from querychat import *
express.QueryChatIt's the same reason why we include reactive, ui, etc in shiny/__init__.py...
| client: chatlas.Chat | ||
|
|
||
|
|
||
| def init_impl( |
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.
Are you using this pattern -- having QueryChat methods shell out to _impl functions -- to maintain the deprecated functions or because it's a structure you prefer?
If it's primarily about the deprecated functions, I'm inclined to say that we should just have a clean break and do a hard deprecation immediately.
- The new syntax is so much better; I want to get people into the new flow as quickly as possible.
- Deprecation warnings are easy to miss; the breaking change forces code upgrades. People can pin to previous versions if they're not ready to migrate yet.
- We can write up a migration guide now as part of this PR. We tend to rely on deprecation warnings to guide through changes, but I think as a user it's a better experience to be pointed to a page that describes the changes, why they happened and how to migrate.
- We can keep the deprecated functions as shells that error out and point to that article.
Overall, I think that a clean break from the old API would be better for us in maintaining the code as well as for our users.
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.
Honestly, I started with soft deprecation since it seemed like the least controversial approach, but you're right that hard deprecating simplifies things significantly enough that I'm up for it 👍.
See 732d699
QueryChat() APIQueryChat() API; hard deprecate old API
b106177 to
25ed22f
Compare
25ed22f to
732d699
Compare
The PR deprecates essentially the entire current (functional) API in favor of a class-based approach (namely
QueryChat). The new API looks something like this:Express
Core
Closes #97
TODO