-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Implement an OCP API for Context Chat #52852
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
Comments
Hey everyone, I think it's ok to leave the APIs in the ContextChat namespace for the following reasons:
I see the following downsides to adding these APIs in server, now:
Which problems do you foresee by leaving things as they are? |
With the same rules as OCP?
Yes, and it's really bad. We should avoid it as much as possible and that we are already in this situation is not a justification for making it worse.
You can make the OCA interface extend the OCP interface, then it's easy to migrate in other apps and you don't have duplicate and eventually out-of-sync code. In my opinion we should have this in NCU first, as I have no idea how stable you are really going to make it. In general this might be the path we chose for every private API of apps: OCA -> NCU -> OCP |
The API are not in nextcloud/ocp composer package, so the tooling of the apps, psalm and such, will not find the classes and complain. That means applications will have to use stubs instead. It is indeed a recurring problem, quite frankly personaly I would be for allowing inter-app dependency and support it properly. If we go this way you could have a nextcloud/context-chat-api or something to be used by depending applications the same way nextcloud/ocp is used. |
I think that would improve the current situation for apps like Circles, but we should not go this route for apps that could still be switched to have their interface in OCP. |
Which would be in OCP:
This would be related to: #4531 |
I would rather have a generalized standard for app public api's (something like Moving api's into |
I think we should go "forward" and allow inter-app dependency when the app marks the class/app/method/interface/… with a
I'm happy to draft the attribute and Demo it on Talk |
👍 for using attributes, might also make sense to add similar ones to |
Separate from the "api stability" is the question around the dependency itself. We should probably have a way to mark those ( We can then automatically (after user confirmation) install any required app to prevent user confusion about why stuff isn't working after they enable an app. |
|
I'll volunteer for looking into tooling to create stubs etc once we've settled on how to annotate things. |
@icewind1991 I've already written some scripts to generate stubs and used it successfully in groupfolders and circles: https://github.com/provokateurin/php-stubs-updater |
Uh oh!
There was an error while loading. Please reload this page.
How to use GitHub
Is your feature request related to a problem? Please describe.
The context_chat app PHP classes in the
OCA\ContextChat\Public
namespace as well as theContentProviderRegisterEvent
event are supposedly available for other apps to use. But since they are still under the app's private namespace, this is not exactly best practice and may cause dependency problems later down the road, when more and more apps will be implemented as context chat providers.Describe the solution you'd like
Move the aforementioned classes to the server repository under the
OCP\ContextChat
namespace instead, and update the developer documentation accordingly.Describe alternatives you've considered
(none)
Additional context
Some initial work was done for the Mail app but needs significant changes (note the
UndefinedClass
errors from Psalm):nextcloud/mail#11150
cc @kyteinsky @marcelklehr @ChristophWurst
The text was updated successfully, but these errors were encountered: