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

Update AutoRest version to 47b422a4 #19959

Closed

Conversation

Mohit-Chakraborty
Copy link
Contributor

Most significant changes included here are from -

Add CustomEnityLookup skill and DocumentExtraction skill
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

I see some changes that violate guidelines.

/// <summary> Changes the default fuzzy edit distance value for this entity. It can be used to change the default value of all aliases fuzzyEditDistance values. </summary>
public int? DefaultFuzzyEditDistance { get; set; }
/// <summary> An array of complex objects that can be used to specify alternative spellings or synonyms to the root entity name. </summary>
public IList<CustomEntityAlias> Aliases { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I know we've talked about it, but we haven't made this change yet, have we? I.e. collection properties shouldn't be settable.

Copy link
Member

Choose a reason for hiding this comment

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

Per internal discussion, these shouldn't be settable since they are collection properties. May be a bug in the generator. /cc @ShivangiReja @pakrym

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should be the resolution?
An update to the generator followed by code regen?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure this property is nullable so we are forced to make it settable.

Copy link
Member

Choose a reason for hiding this comment

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

In Search we already had that issue and resolved it without making them settable. Previously we used the NullAsEmpty attribute property (or something like that), but I don't remember where we landed finally off hand. Pretty sure it wasn't settable collection properties. @Mohit-Chakraborty , I recommend looking at existing models with collection properties and see what we're doing.

@Mohit-Chakraborty
Copy link
Contributor Author

cc: @tg-msft

@Mohit-Chakraborty
Copy link
Contributor Author

Closing this PR as we are updating to a new spec.
Please take a look at the new PR - #20078

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.

3 participants