Add update method instead of setters for CSharpType #6035
Conversation
microsoft-github-policy-service
bot
added
the
emitter:client:csharp
live1206
requested review from
m-nash,
JoshLove-msft and
jorgerangel-msft
as code owners
| /// <summary> | ||
| /// Gets and sets the name of the type. | ||
| /// </summary> | ||
| public string Name |
There was a problem hiding this comment.
Why are we making this one settable? Isn't it enough that CSharpType.Name is settable? I think we should do away with this public property similar to what we did with Namespace.
There was a problem hiding this comment.
I don't think Name can be simply removed like Namespace, it has been widely used, updated to take _type.Name as preference.
...ages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/Providers/TypeProvider.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Gets and sets the declaration modifiers for the type. | ||
| /// </summary> | ||
| public TypeSignatureModifiers DeclarationModifiers |
There was a problem hiding this comment.
Do we still need the setter here?
|
|
||
| private string? _relativeFilePath; | ||
|
|
||
| public string Name => _name ??= CustomCodeView?.Name ?? BuildName(); | ||
| public string Name => |
There was a problem hiding this comment.
We should remove this property since it will be on the CSharpType.
There was a problem hiding this comment.
Name has been copied to CSharpType
The source of truth is here, and the Name property has been used everywhere, and the original value comes from BuildName() in TypeProvider, we need _name to hold it.
There was a problem hiding this comment.
Ok then we can revert the change here.
There was a problem hiding this comment.
It's fine to retain the property for usability, but there should only be one source for Name -> Type.Name. It should mirror how it works for namespace.
There was a problem hiding this comment.
But then, we need to update both TypeProvider.Name and CSharpType.Name since the name got copied into CSharpType since we don't hold Implementation in CSharpType.
There was a problem hiding this comment.
We should do the same thing we do with namespace where we pass it into the internal ctor of CSharpType.
There was a problem hiding this comment.
OK then, I need to update the constructor of CSharpType to name to break the circular dependency
instead of
| /// <summary> | ||
| /// Gets or sets the name of the type. | ||
| /// </summary> | ||
| public string Name { get; set; } |
There was a problem hiding this comment.
Can we instead add an Update method?
There was a problem hiding this comment.
#6035 (comment)
OK. Thought you were saying I can still add the setter, we will use update method later.
But, if we decide to use the update method instead. Sure.
…od for CSharpType
live1206
changed the title
| if (name != null) | ||
| { | ||
| Name = name; | ||
| FullyQualifiedName = $"{Namespace}.{name}"; |
There was a problem hiding this comment.
nit: we can just make this property be a lambda getter so we don't have to set it all over the place.
These changes are needed for Azure/azure-sdk-for-net#48362