Skip to content

Remove setters from collection properties#240

Merged
ShivangiReja merged 3 commits into
joseharriaga:mainfrom
ShivangiReja:Remove_Collection_Setter
Sep 11, 2024
Merged

Remove setters from collection properties#240
ShivangiReja merged 3 commits into
joseharriaga:mainfrom
ShivangiReja:Remove_Collection_Setter

Conversation

@ShivangiReja
Copy link
Copy Markdown
Collaborator

We have removed most of the setters from the collection properties in this PR. This PR removes the remaining setters from the public collection properties.

Note: A setter is only generated for nullable collections. For more details, please see this conversation.

}
public class AssistantModificationOptions : IJsonModel<AssistantModificationOptions>, IPersistableModel<AssistantModificationOptions> {
public IList<ToolDefinition> DefaultTools { get; set; }
public IList<ToolDefinition> DefaultTools { get; }
Copy link
Copy Markdown
Owner

@joseharriaga joseharriaga Sep 11, 2024

Choose a reason for hiding this comment

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

Thank you, Shivangi! After this change:

  • AssistantModificationOptions.DefaultTools
  • CodeInterpreterToolResources.FileIds
  • FileSearchToolResources.VectorStoreIds
  • VectorStoreCreationOptions.FileIds

The only collection properties left that have a setter are the following:

  • MessageCreationOptions.Attachments
  • AssistantCreationOptions.Metadata
  • AssistantModificationOptions.Metadata
  • MessageCreationOptions.Metadata
  • MessageModificationOptions.Metadata
  • RunModificationOptions.Metadata
  • ThreadCreationOptions.Metadata
  • ThreadModificationOptions.Metadata
  • VectoreStoreCreationOptions.Metadata
  • VectorStoreModificationOptions.Metadata

Paging in @trrwilson because we talked about something related a while ago. The properties above are getting a setter from the generator because they are explicitly specified as nullable in the TypeSpec (which itself comes from the swagger spec). Now, in basically every part of the REST API that I have encountered so far, setting a nullable property to null does not imply anything special and it's equivalent to not setting it at all. Assuming null doesn't mean anything special here either, I wonder if we should add a simple change to customize these properties to remove the setter in favor of consistency and a simpler API.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@ShivangiReja : I have approved this PR, and you can merge it. If we decide to revise these other properties, they can go in a separate PR. 😊

@ShivangiReja ShivangiReja merged commit 7a3050b into joseharriaga:main Sep 11, 2024
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.

2 participants