Skip to content

Serialize configuration and tspCodeModel from swagger #5289

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

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ArcturusZhang
Copy link
Member

Description

Add your description here!

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

@ArcturusZhang ArcturusZhang changed the title Serialize tspCodeModel from swagger Serialize configuration and tspCodeModel from swagger Apr 11, 2025
@@ -10,9 +12,31 @@ internal class AzurePluginTarget
{
public static async Task ExecuteAsync(GeneratedCodeWorkspace project, InputNamespace inputNamespace)
{
// TODO: serialize configuration.json and serialize inputNamespace to tspCodeModel.json
// normalize the output folder
var outputFolder = NormalizeOutputFolder(Configuration.OutputFolder);
Copy link
Member Author

Choose a reason for hiding this comment

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

this should be unnecessary, I will resolve this when the majority feature of this is done

Copy link
Member

@live1206 live1206 left a comment

Choose a reason for hiding this comment

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

Overall, it LGTM, what's the goal of this PR?
I think we don't have to implement all the details of serialization within this single PR.

@@ -5,7 +5,7 @@

namespace AutoRest.CSharp.Common.Input;

internal record InputEnumTypeValue(string Name, object Value, string? Summary, string? Doc)
internal abstract record InputEnumTypeValue(string Name, object Value, string? Summary, string? Doc)
Copy link
Member

Choose a reason for hiding this comment

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

think we should take this change to MTG to avoid instantiation of this.

Copy link
Member Author

@ArcturusZhang ArcturusZhang Apr 15, 2025

Choose a reason for hiding this comment

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

yep - this part is done in a really messy way, as well as in MTG.
I think I would like to figure it out in MTG in the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

and to clarify, I think "using the input library from MTG" should not be part of our plan - it will be a huge change and we might encounter various of issues. At least in short time, we do not need to consider to use the types from MTG - having these types defined in this library actually has its advantages.

writer.WriteStartObject();

// kind
writer.WriteString("kind", UtcDateTimeKind); // TODO -- currently the two different kinds have exactly the same properties, therefore here we no longer have the ability to distinguish
Copy link
Member

Choose a reason for hiding this comment

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

what's the plan for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is possible that we make this class abstract, and make UtcDateTimeType and OffsetDateTimeType to derive from it, like what we have done for InputType.

Copy link
Member Author

Choose a reason for hiding this comment

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

And I think we should also do the same in MTG.

@ArcturusZhang
Copy link
Member Author

ArcturusZhang commented Apr 15, 2025

Overall, it LGTM, what's the goal of this PR? I think we don't have to implement all the details of serialization within this single PR.

Yeah it might be too much of code changes if I do all of them. But if I did not do all of them, we do not have a way to test it.
I would be fine if I just stop right now - because in the current state of this PR, everything other than the clients has been serialized.

var outputFolder = NormalizeOutputFolder(Configuration.OutputFolder);
// write the configuration.json
var configurationFilepath = Path.Combine(outputFolder, "Configuration.json");
await File.WriteAllTextAsync(configurationFilepath, Configuration.SaveConfiguration());
Copy link
Member

Choose a reason for hiding this comment

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

Let's create a tracking item to track converting the configuration. Some configuration settings aren't valid in MTG. There are also some renames like library-name changed to package-name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here: #5293

@@ -10,9 +12,31 @@ internal class AzurePluginTarget
{
public static async Task ExecuteAsync(GeneratedCodeWorkspace project, InputNamespace inputNamespace)
{
// TODO: serialize configuration.json and serialize inputNamespace to tspCodeModel.json
// normalize the output folder
Copy link
Member

Choose a reason for hiding this comment

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

Lets update the signature to take in a codemodel and do this conversion code here https://github.com/Azure/autorest.csharp/pull/5289/files#diff-42f99a04ab6ed3698d0a6aa3834bdf524f9d13d31aaaef67ab8476ade2c8927eR49-R54 inside the target itself

@@ -10,9 +12,31 @@ internal class AzurePluginTarget
{
public static async Task ExecuteAsync(GeneratedCodeWorkspace project, InputNamespace inputNamespace)
{
// TODO: serialize configuration.json and serialize inputNamespace to tspCodeModel.json
// normalize the output folder
var outputFolder = NormalizeOutputFolder(Configuration.OutputFolder);
Copy link
Member

Choose a reason for hiding this comment

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

On this line of code here https://github.com/Azure/autorest.csharp/pull/5289/files#diff-42f99a04ab6ed3698d0a6aa3834bdf524f9d13d31aaaef67ab8476ade2c8927eR51 we will need to call dataplane if its dataplane and mgmt if its mgmt.

Summary: string.Empty,
Doc: choiceValue.Language.Default.Description,
Value: value!);
var name = choiceValue.Language.Default.Name;
Copy link
Member

Choose a reason for hiding this comment

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

Lets consider making this a singleton

Copy link
Member Author

Choose a reason for hiding this comment

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

Created this issue to track: #5295

@@ -65,5 +66,30 @@ public static InputDateTimeType CreateDateTimeType(ref Utf8JsonReader reader, st
}
return dateTimeType;
}

public override void Write(Utf8JsonWriter writer, InputDateTimeType value, JsonSerializerOptions options)
Copy link
Member

Choose a reason for hiding this comment

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

Lets add a tracking item to consider using the MTG.Input library instead of having its own.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here: #5292

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.

3 participants