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

Maorleger/cosmos refactor #33365

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Maorleger/cosmos refactor #33365

wants to merge 4 commits into from

Conversation

maorleger
Copy link
Member

@maorleger maorleger commented Mar 12, 2025

Just a few options to resolve https://spa.apiview.dev/review/78434da7bab841c583864e3a27d3c043?activeApiRevisionId=6de9f9740782489f85b4861c3a837da7&nId=@azure%2Fcosmos!EncryptionKeyWrapMetadata%23value:member

Helpful to review each option separately:

  • option 1 shows one possibility. From a usage perspective, the intellisense is not great when adding a number without the dbType param but it's worth exploring
  • option 2 shows a hybrid of overloads and a separate function for numeric parameters that need to be disambiguated
  • option 3 removes the overloads and exposes the union type directly

@amanrao23 a few options for you to review. I prefer the second (overload for "normal" parameters)

@joheredi would love your take on this as well sometime today - is one better from a JS / idiomatic perspective? They're trying to ship by friday

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

@azure/cosmos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants