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.

Sign up for GitHub

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

Expose public setter of namespace #5928

Merged
merged 14 commits into from
Feb 12, 2025
Merged

Expose public setter of namespace #5928

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
928dce2
Add abstraction for setting namespace of Model and Enum.
live1206 Feb 10, 2025
15efe67
Add abstrction to set model factory namespace
live1206 Feb 10, 2025
8a5c0df
Merge remote-tracking branch 'origin/main' into update-provider-names…
live1206 Feb 10, 2025
b347766
revert namespace abstraction, add public setter instead
live1206 Feb 11, 2025
4d1e781
expose EnumProvider to let sub-plugin identify
live1206 Feb 11, 2025
cc4d378
expose ModelFactoryProvider
live1206 Feb 11, 2025
2b80faf
Merge remote-tracking branch 'origin/main' into update-provider-names…
live1206 Feb 11, 2025
d9798a6
remove Namespace from TypeProvider, use TypeProvider.Type.Namespace f…
live1206 Feb 11, 2025
b058ac6
expose ExtensibleEnumSerializationProvider and FixedEnumSerialization…
live1206 Feb 12, 2025
98b2501
remove duplicate instances of model factory
live1206 Feb 12, 2025
1972665
remove GenerateModelFactory from configuration
live1206 Feb 12, 2025
28df98a
lift ModelFactoryProvider instance to TypeProvider instead of the sta…
live1206 Feb 12, 2025
2040566
cleanup
live1206 Feb 12, 2025
f54b2f3
move ModelFactoryProvider instance to OutputLibrary
live1206 Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/OutputLibrary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.TypeSpec.Generator.Providers;
Expand All @@ -16,6 +17,8 @@ public IReadOnlyList<TypeProvider> TypeProviders
internal set => _typeProviders = value;
}

internal Lazy<ModelFactoryProvider> ModelFactory { get; } = new(() => new ModelFactoryProvider(CodeModelPlugin.Instance.InputLibrary.InputNamespace.Models));
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to use Lazy here. We can just cache the value in private field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshLove-msft
If you are saying like

internal ModelFactoryProvider ModelFactory { get; } = new ModelFactoryProvider(CodeModelPlugin.Instance.InputLibrary.InputNamespace.Models);

This will not work, because the field will be initialized while creating OutputLibrary instance, and it need CodeModelPlugin.Instance to be created. But CodeModelPlugin.Instance depends on the instance of OutputLibrary, we have a circular dependency here.

If you are saying like

internal ModelFactoryProvider ModelFactory { get; } = _field ??= new ModelFactoryProvider(CodeModelPlugin.Instance.InputLibrary.InputNamespace.Models);

why is this better than using Lazy?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm suggesting that we use a private field to cache the value in the getter rather than using Lazy. Lazy is useful when there are concurrency concerns to ensure that the value is initialized only once. We don't have such concerns here, and there is a performance penalty to using Lazy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought it's a good thing to be thread-safe ready. But if that's not what we need for now, sure.


private static TypeProvider[] BuildEnums()
{
var input = CodeModelPlugin.Instance.InputLibrary.InputNamespace;
Expand Down Expand Up @@ -79,9 +82,9 @@ .. BuildModelFactory()
];
}

private static TypeProvider[] BuildModelFactory()
private TypeProvider[] BuildModelFactory()
{
var modelFactory = CodeModelPlugin.Instance.TypeFactory.ModelFactory.Value;
var modelFactory = ModelFactory.Value;
return modelFactory.Methods.Count > 0 ? [modelFactory] : [];
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...sharp/generator/Microsoft.TypeSpec.Generator/src/PostProcessing/GeneratedCodeWorkspace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ internal static Project AddDirectory(Project project, string directory, Func<str
/// </summary>
public async Task PostProcessAsync()
{
var modelFactory = CodeModelPlugin.Instance.TypeFactory.ModelFactory.Value;
var modelFactory = CodeModelPlugin.Instance.OutputLibrary.ModelFactory.Value;
var postProcessor = new PostProcessor(
[.. CodeModelPlugin.Instance.TypeFactory.UnionTypes, .. CodeModelPlugin.Instance.TypesToKeep],
modelFactoryFullName: $"{modelFactory.Type.Namespace}.{modelFactory.Name}");
Expand Down
2 changes: 0 additions & 2 deletions packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/TypeFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ public class TypeFactory
private HashSet<string>? _unionTypes;
internal HashSet<string> UnionTypes => _unionTypes ??= [];

internal Lazy<ModelFactoryProvider> ModelFactory { get; } = new(() => new ModelFactoryProvider(CodeModelPlugin.Instance.InputLibrary.InputNamespace.Models));

protected internal TypeFactory()
{
}
Expand Down
12 changes: 6 additions & 6 deletions ...r/Microsoft.TypeSpec.Generator/test/Providers/ModelFactories/ModelFactoryProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ public ModelFactoryProviderTests()
[Test]
public void SkipInternalModels()
{
var modelFactory = _instance.TypeFactory.ModelFactory.Value;
var modelFactory = _instance.OutputLibrary.ModelFactory.Value;
Assert.AreEqual(ModelList.Length - ModelList.Where(m => m.Access == "internal").Count(), modelFactory.Methods.Count);
}

[Test]
public void ListParamShape()
{
var modelFactory = _instance.TypeFactory.ModelFactory.Value;
var modelFactory = _instance.OutputLibrary.ModelFactory.Value;
var models = ModelList.Select(CodeModelPlugin.Instance.TypeFactory.CreateModel);
foreach (var model in models)
{
Expand All @@ -55,7 +55,7 @@ public void ListParamShape()
[Test]
public void DictionaryParamShape()
{
var modelFactory = _instance.TypeFactory.ModelFactory.Value;
var modelFactory = _instance.OutputLibrary.ModelFactory.Value;
var models = ModelList.Select(CodeModelPlugin.Instance.TypeFactory.CreateModel);
foreach (var model in models)
{
Expand All @@ -78,7 +78,7 @@ public void DictionaryParamShape()
[Test]
public void DiscriminatorEnumParamShape()
{
var modelFactory = _instance.TypeFactory.ModelFactory.Value;
var modelFactory = _instance.OutputLibrary.ModelFactory.Value;
var models = ModelList.Select(CodeModelPlugin.Instance.TypeFactory.CreateModel);
foreach (var model in models)
{
Expand All @@ -100,7 +100,7 @@ public void DiscriminatorEnumParamShape()
[Test]
public void AdditionalPropertiesParamShape()
{
var modelFactory = _instance.TypeFactory.ModelFactory.Value;
var modelFactory = _instance.OutputLibrary.ModelFactory.Value;
var models = ModelList.Select(CodeModelPlugin.Instance.TypeFactory.CreateModel);
foreach (var model in models)
{
Expand All @@ -124,7 +124,7 @@ public void AdditionalPropertiesParamShape()
[Test]
public void ModelFactoryName()
{
var modelFactory = _instance.TypeFactory.ModelFactory.Value;
var modelFactory = _instance.OutputLibrary.ModelFactory.Value;
Assert.AreEqual("SampleNamespaceModelFactory", modelFactory.Name);
}

Expand Down
Loading