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

Expose public setter of namespace #5928

Merged
merged 14 commits into from
Feb 12, 2025

Conversation

live1206
Copy link
Contributor

@live1206 live1206 commented Feb 10, 2025

After the tsp namespace feature implementation in #5443, the namespace of model, enum and modelFactory have been changed from previous XXX.Models to XXX.
For Azure plugin, we would like the namespace for model and enum to be XXX.Models.

@microsoft-github-policy-service microsoft-github-policy-service bot added the emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp label Feb 10, 2025
@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Microsoft.Generator.CSharp

Copy link
Member

@ArcturusZhang ArcturusZhang left a comment

Choose a reason for hiding this comment

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

LGTM.
In the meantime, since we have more and more callbacks on the typefactory, it occurs to me that do we need to consider to categorize these methods into different classes?
For instance, for these namespace related ones, maybe we could have something like

public class TypeFactory
{
public virtual NamespaceNamingStrategy NamespaceNamingStrategy {get;} = NamespaceNamingStrategy.Default;
}

and these methods could go to the NamespaceNamingStrategy class where could also hold those GetCleanNamespace methods.

@live1206 live1206 changed the title Add abstraction for setting namespace of Model and Enum Expose public setter of namespace Feb 11, 2025
@live1206 live1206 added this pull request to the merge queue Feb 12, 2025
Merged via the queue into microsoft:main with commit ffef215 Feb 12, 2025
21 checks passed
@live1206 live1206 deleted the update-provider-namespace branch February 12, 2025 07:42
@@ -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.

dmnorc pushed a commit to dmnorc/typespec that referenced this pull request Feb 18, 2025
After the tsp namespace feature implementation in
microsoft#5443, the namespace of model,
enum and modelFactory have been changed from previous `XXX.Models` to
`XXX`.
For Azure plugin, we would like the namespace for model and enum to be
`XXX.Models`.

- Expose public setter of namespace, use visitor to update namespace of
TypeProvider in Azure plugin.
https://github.com/Azure/azure-sdk-for-net/pull/48197/files#diff-a6219e0492ffcf33fd4bb03be26f1e6bc04a84ee1e2723b4d58d5adbcdb031ef
- Expose provider types to let sub-plugin identify.
- Remove duplicated instance of ModelFactoryProvider
- Remove Namespace property from CSharp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants