-
Notifications
You must be signed in to change notification settings - Fork 173
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
base: main
Are you sure you want to change the base?
Serialize configuration and tspCodeModel from swagger #5289
Conversation
@@ -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); |
There was a problem hiding this comment.
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
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
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. |
var outputFolder = NormalizeOutputFolder(Configuration.OutputFolder); | ||
// write the configuration.json | ||
var configurationFilepath = Path.Combine(outputFolder, "Configuration.json"); | ||
await File.WriteAllTextAsync(configurationFilepath, Configuration.SaveConfiguration()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here: #5292
Description
Add your description here!
Checklist
To ensure a quick review and merge, please ensure:
Ready to Land?