-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix: Remove grounding module related types #270
Conversation
.changeset/remove-grounding.md
Outdated
--- | ||
'@sap-ai-sdk/orchestration': minor | ||
--- | ||
[Compatibility Note] Remove grounding module related types, which should not be used. |
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.
[q] Do we really need a compatibility note? This was never part of the public api.
The fact that we also expose ModuleConfigs
in index.ts
is incorrect (unrelated to this PR).
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.
For SDK user, I agree, it's not relevant, if they only use public APIs, so I deleted the compatibility notes.
I put it there, to make it transparent for those who noticed our grounding related code.
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.
will work on ModuleConfigs
in a different PR.
@@ -9,7 +9,7 @@ import type { AzureContentSafety } from './azure-content-safety.js'; | |||
*/ | |||
export type AzureContentSafetyFilterConfig = { | |||
/** | |||
* String represents name of the filter provider. | |||
* String represents name of the filter provider |
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.
[pp] I think these changes are fine for now. But it's better to have proper linted code. I would like to adjust our eslint config to fix such issues (and only disable consistent-typedefinitions rule).
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.
LGTM. Please consider the comment about having the changeset.
Removed already 😄 |
Context
This PR contains:
Definition of Done