Skip to content

Remove the IncludeDeserializer constraint in model factory#3574

Closed
ArcturusZhang wants to merge 13 commits intoAzure:feature/v3from
ArcturusZhang:temp-fix-for-mgmt-hlc-internal-ctor
Closed

Remove the IncludeDeserializer constraint in model factory#3574
ArcturusZhang wants to merge 13 commits intoAzure:feature/v3from
ArcturusZhang:temp-fix-for-mgmt-hlc-internal-ctor

Conversation

@ArcturusZhang
Copy link
Copy Markdown
Member

@ArcturusZhang ArcturusZhang commented Jul 10, 2023

Fixes #3181
Fixes #3406

Description

Superseded by #3434

Now in HLC/mgmt, all models will have an internal ctor if it is different from the initialization ctor.

Removes the constraint in model factory that a model must have deserializer to be included in model factory.

There should be two kind of changes included in the regen:

  1. added an internal ctor that takes all properties as parameters.
  2. added some new entries in the model factory.

Checklist

To ensure a quick review and merge, please ensure:

  • The PR has a understandable title and description explaining the why and what.
  • The PR is opened in draft if not ready for review yet.
    • If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
  • The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

  • Build is completely green
    • Submissions with test failures require tracking issue and approval of a CODEOWNER
  • At least one +1 review by a CODEOWNER
  • All -1 reviews are confirmed resolved by the reviewer
    • Override/Marking reviews stale must be discussed with CODEOWNERS first

…ry when it is not output, we will need the unknown schema when it does not have output
@ArcturusZhang
Copy link
Copy Markdown
Member Author

Still need to resolve the issue that postprocess will remove the Unknown if it is only used in model factory.

Comment on lines -54 to -60
//Since the unknown type is used for deserialization only we don't need to create if its an input only model
var hasXCsharpUsageOutput = !actualBaseSchema.Extensions?.Usage?.Contains("output", StringComparison.OrdinalIgnoreCase);
if (!actualBaseSchema.Usage.Contains(SchemaContext.Output) &&
!actualBaseSchema.Usage.Contains(SchemaContext.Exception) &&
(!hasXCsharpUsageOutput.HasValue ||
hasXCsharpUsageOutput.Value))
return;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is removed because since now we will generate model factory entries for the models without output usage, when this model is the base model with a discriminator and is abstract, we will need the unknown to construct the instance.
If this unknown is generated and never used anywhere (for instance, it does not meet the criteria of model factory), the post process will take care of it and remove it as well

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But now we have a bug on the post process that it is removed anyway.

@ArcturusZhang ArcturusZhang marked this pull request as ready for review July 13, 2023 07:02
@ArcturusZhang ArcturusZhang marked this pull request as draft July 13, 2023 07:03
@ArcturusZhang ArcturusZhang marked this pull request as ready for review July 13, 2023 10:26
@ArcturusZhang
Copy link
Copy Markdown
Member Author

@m-nash
Could you please have a review on this PR?
This PR adds internal ctor for every model if it previously does not have one. We cannot really predict whether a model would be used in the model factory when writing its constructors therefore I am writing an internal ctor for all of the models. Since the ctor is always internal, it does not harm anything.
It is possible to remove the unused internal ctors in post processors, but finding references of a symbol is extremely time-consuming therefore I personally prefer we just let them be there.

@m-nash
Copy link
Copy Markdown
Member

m-nash commented Jul 19, 2023

@m-nash Could you please have a review on this PR? This PR adds internal ctor for every model if it previously does not have one. We cannot really predict whether a model would be used in the model factory when writing its constructors therefore I am writing an internal ctor for all of the models. Since the ctor is always internal, it does not harm anything. It is possible to remove the unused internal ctors in post processors, but finding references of a symbol is extremely time-consuming therefore I personally prefer we just let them be there.

Looks good but lets get the ci cleaned up and green first.

@ArcturusZhang
Copy link
Copy Markdown
Member Author

@m-nash Could you please have a review on this PR? This PR adds internal ctor for every model if it previously does not have one. We cannot really predict whether a model would be used in the model factory when writing its constructors therefore I am writing an internal ctor for all of the models. Since the ctor is always internal, it does not harm anything. It is possible to remove the unused internal ctors in post processors, but finding references of a symbol is extremely time-consuming therefore I personally prefer we just let them be there.

Looks good but lets get the ci cleaned up and green first.

This is a bit out-of-date now because my recent focus is on the confident level PR. I will fix this and CIs in the corresponding regen PR as well. I will let you know when this ready for review again.

@ArcturusZhang
Copy link
Copy Markdown
Member Author

Close this in favor of #3434 because it contains everything in this PR

@ArcturusZhang ArcturusZhang deleted the temp-fix-for-mgmt-hlc-internal-ctor branch August 3, 2023 02:06
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