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

Adding IsRequired property to all parameters #1457

Closed

Conversation

teresaqhoang
Copy link
Contributor

Motivation and Context

Extending parameter objects to include an IsRequired field for better planner resiliency.

Description

Also, includes some logic changes to Copilot PlanViewer to show INPUT value.

Contribution Checklist

@teresaqhoang teresaqhoang added PR: ready for review All feedback addressed, ready for reviews planner Anything related to planner or plans kernel Issues or pull requests impacting the core kernel samples labels Jun 13, 2023
@teresaqhoang teresaqhoang self-assigned this Jun 13, 2023
@github-actions github-actions bot added .NET Issue or Pull requests regarding .NET code kernel.core labels Jun 13, 2023
/// Whether the parameter is required.
/// </summary>
public bool IsRequired { get; set; } = true;

/// <summary>
/// Creates a parameter view, using information from an instance of this class.
/// </summary>
Copy link
Member

Choose a reason for hiding this comment

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

#1195 is proposing some changes in the area. Might use this to help speed up things over there.

adrianwyatt
adrianwyatt previously approved these changes Jun 14, 2023
Copy link
Contributor

@adrianwyatt adrianwyatt left a comment

Choose a reason for hiding this comment

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

LGTM - but I'm curious about using string.join as well.

@teresaqhoang
Copy link
Contributor Author

Closing in favor of Stephen's PR: #1195

Will an issue to re-evaluate how we specify required / optional parameters to the Planner. It seems like Stephen's PR will handle native functions well, where all parameters are required but have a DefaultValue of null when optional. But some native functions specify (optional) in the parameter description, whereas we're specifying (required) in the description of rest api parameters when necessary. The AI could get confused for parameters with descriptions that don't contain either postfix, so we should pick one flag or the other for the planner's functionView.

@teresaqhoang teresaqhoang deleted the required-function-params branch December 13, 2023 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code planner Anything related to planner or plans PR: ready for review All feedback addressed, ready for reviews
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants