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

Add explicit | undefined for LanguageModelChatTool #241502

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Feb 21, 2025

Fixes #241285

This property can either be unset or set to undefined

Fixes microsoft#241285

This property can either be unset of set to undefined
@mjbvz mjbvz requested a review from jrieken as a code owner February 21, 2025 17:45
@mjbvz mjbvz self-assigned this Feb 21, 2025
@mjbvz mjbvz requested a review from roblourens February 21, 2025 17:45
@mjbvz mjbvz added this to the February 2025 milestone Feb 21, 2025
@mjbvz mjbvz enabled auto-merge February 21, 2025 17:45
Copy link

⚠️ This PR originates from a fork. Due to security restrictions, pipelines from forks are no longer triggered automatically. Learn more.

If the changes appear safe, you can manually trigger the pipeline by commenting /AzurePipelines run.

@roblourens
Copy link
Member

Why do you have to that explicitly? Isn't this essentially equivalent?

@mjbvz
Copy link
Collaborator Author

mjbvz commented Feb 24, 2025

@roblourens Normally yes, but with exactOptionalPropertyTypes you have to specify if an optional property can be set to undefined instead of just being unset. This can be important as code sometimes treats something like foo({ abc: undefined }) as being different from foo({ }) where abc is omitted entirely. In our apis though we almost always treat them as being the same and this change makes that explicit

@roblourens
Copy link
Member

Feel free to merge it if this is needed but why don't other optional properties in vscode.d.ts need this then?

@mjbvz
Copy link
Collaborator Author

mjbvz commented Feb 24, 2025

We possibly should at some point but it's kind of a pain for limited real world benefit in most cases. This specific case is more important since you have two distinct types that were meant to be compatible (LanguageModelToolInformation and LanguageModelChatTool) but weren't because one used ? and the other | undefined

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