Skip to content
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

feat: Add descriptive constant for Azure content filter configuration #485

Merged

Conversation

KavithaSiva
Copy link
Contributor

@KavithaSiva KavithaSiva commented Jan 24, 2025

Context

Closes SAP/ai-sdk-js-backlog#181.

What this PR does and why it is needed

This PR adds self descriptive constants for improving Azure content filter configuration.

Screenshot 2025-02-03 at 05 49 20

Autocompletion works with the string key values of azureFilterThreshold, the numeric allowed values don't come up in the hints.
These constants can only be used in the new convenience function introduced buildAzureContentSafetyFilter()
The old convenience function buildAzureContentFilter() still allows using numeric values.

@KavithaSiva KavithaSiva marked this pull request as ready for review January 24, 2025 11:11
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/src/index.ts Outdated Show resolved Hide resolved
/**
* A descriptive constant for Azure content safety filter threshold.
*/
export const AzureFilterThreshold = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q] Does this need to be exported?
[req] Please follow styleguide for reusable const https://github.com/SAP/cloud-sdk-js/blob/main/STYLEGUIDE.md#use-camel-case-for-variable-names-

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like the naming of the constants, honestly I would vote for using reasonable names instead of this mess, even if it breaks with the other sdks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q] Does this need to be exported?

Yes, because I use it to fetch the corresponding integer value to construct the generated azure content filter type in the filtering convenience function.

[req] Please follow styleguide for reusable const https://github.com/SAP/cloud-sdk-js/blob/main/STYLEGUIDE.md#use-camel-case-for-variable-names-

Renamed it to azureFilterThreshold.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like the naming of the constants, honestly I would vote for using reasonable names instead of this mess, even if it breaks with the other sdks.

Launchpad also uses similar threshold levels, and I guess these constants were picked by the other SDK's to also align with launchpad.
So, I would leave this the same for now, although I agree the const names are not very intuitive.

packages/orchestration/src/orchestration-types.ts Outdated Show resolved Hide resolved
packages/orchestration/src/orchestration-types.ts Outdated Show resolved Hide resolved
packages/orchestration/src/util/filtering.ts Outdated Show resolved Hide resolved
KavithaSiva and others added 12 commits January 29, 2025 14:27
…ive-const-content-filter

# Conflicts:
#	packages/orchestration/README.md
#	packages/orchestration/src/orchestration-client.test.ts
#	packages/orchestration/src/orchestration-completion-post-request.test.ts
#	packages/orchestration/src/util/filtering.test.ts
#	packages/orchestration/src/util/filtering.ts
#	packages/orchestration/src/util/request-config.test.ts
#	sample-code/src/orchestration.ts
@KavithaSiva KavithaSiva requested a review from deekshas8 February 3, 2025 04:47
@KavithaSiva KavithaSiva dismissed deekshas8’s stale review February 3, 2025 09:44

Adapted all code review comments.

Copy link
Contributor

@deekshas8 deekshas8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the export of the AzureFilterThreshold type. Rest LGTM

@deekshas8
Copy link
Contributor

Is the changelog part of the other PR? Ideally it should be part of this commit.

@KavithaSiva
Copy link
Contributor Author

Is the changelog part of the other PR? Ideally it should be part of this commit.

I will add it as part of the other PR, as the function gets introduced there, and only a convenience gets added here.

@KavithaSiva KavithaSiva merged commit 2908711 into refactor-content-filters Feb 3, 2025
6 checks passed
@KavithaSiva KavithaSiva deleted the feat/descriptive-const-content-filter branch February 3, 2025 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants