[CodeGenLib] Differentiate type names from identifying names #1759
Conversation
Motivation: The names of the namespaces/service/methods used in the type names in the generated code can be different from the names identifying the namespaces/service/methods in the IDL. This is why we need to have different properties for them. Methods also have a third name - for the function signatures. Modifications: - Added the new names properties in the ServiceDescriptor and MethodDescriptor from CodeGenerationRequest - Modified the existing tests accordingly - Added new validation methods verifying the generated names (or signature name) - Added tests for the new checks Result: Users will be able to set a identifying name, a generated name (and a signature name for the methods) for the namespaces and services.
| /// Service name. | ||
| public var name: String | ||
|
|
||
| /// The service name used in the generated type names. |
There was a problem hiding this comment.
It's not clear from the docs/name what this is used for or what the expectation is (e.g. upper vs lower case). Presumably it forms the base of names used for a type i.e. "<name>Protocol" etc? I think this is partly fixed by a better name and also with documentation.
Same comment applies to the other generated names.
| } | ||
|
|
||
| // Check that service names are unique within each namespace. | ||
| // Check that service names and service generated names are unique within each namespace. |
There was a problem hiding this comment.
We only call this function with the descriptors grouped by the generated namespace, why do we still need to check the non-generated service names?
I think the only modifications you need to make to your existing checks should be to use the generated names instead of the current ones and to add one more check which ensures that all descriptors are unique.
There was a problem hiding this comment.
So, for the methods, I think it is important to check that the upper case names are unique within a service (because they are enum names inside the service enum), but also that the lowerCase names are unique within a service, as they will be used for function declaration in the same protocols. So, in this case I think it's useful to have 3 checks (the third one is for the descriptor). But for the services I will only check the upperCase names and the descriptors (that they are unique).
| /// Represents the name associated with a namespace, service or a method, in three different formats, | ||
| /// which are used in specific parts of the generated code. There must be only one ``Name`` object | ||
| /// for each namespace, service or method. | ||
| /// | ||
| /// The base name, the generatedUpperCase (and the generatedLowerCase) must be unique for methods | ||
| /// from within the same service and for services within the same namespace. |
There was a problem hiding this comment.
Try to avoid be too specific in documentation, this is just a collection of names in different formats. Limitations like uniqueness should be documented where we use this type in the request (i.e. on the service descriptor name and namespace).
| /// The `generatedUpperCase` name is used as part of generated type names, which begin with a capital letter | ||
| /// and are (partially) using CamelCase. It is using UpperCamelCase in order to follow the Swift naming conventions. | ||
| /// | ||
| /// It is used in the generated code as an enum name and as part of protocol names. | ||
| /// For example, in the generated server code the name of the service protocol | ||
| /// follows this pattern: | ||
| /// `<service_namespace_generatedUpperCaseName>_<service_generatedUpperCaseName>ServiceProtocol`. |
There was a problem hiding this comment.
We don't need to be so specific, we're setting ourselves up to have outdated docs (because we may change the pattern and forget about the docs).
In this case the relevant info is that this name is used in generated code and is expected to be the upper-camel-case version of the base name.
You can then provide an abstract example, e.g. if base is "echo" then generatedUpperCase could be "Echo".
| /// | ||
| /// The base name is also used in the descriptors that identify a specific method or service : | ||
| /// `<service_namespace_baseName>.<service_baseName>.<method_baseName>`. | ||
| public let base: String |
There was a problem hiding this comment.
Properties on structs should be var unless you have a good reason for them not to be.
Making them a let only makes using the type harder:
var foo = Foo(bar: "bar")
foo.bar = "baz" // Not possible if 'bar' is a 'let'
foo = Foo(bar: "baz") // Possible, but more annoying to do than the above, especially if there are multiple properties| ) throws { | ||
| let methodNames = service.methods.map { $0.name } | ||
| var seenNames = Set<String>() | ||
| // Check that the method descriptors are unique, by checking that the base names |
There was a problem hiding this comment.
This function does the same thing three times but with different information. Can you refactor this to avoid the duplication?
| let descriptors = services.map { | ||
| ($0.namespace.base.isEmpty ? "" : "\($0.namespace.base).") + $0.name.base | ||
| } | ||
| if let duplicate = descriptors.hasDuplicates() { | ||
| throw CodeGenError( | ||
| code: .nonUniqueServiceName, | ||
| message: """ | ||
| Services must have unique descriptors. \ | ||
| \(duplicate) is the descriptor of at least two different services. | ||
| """ | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
There's no need to create an intermediate array here, we can just do this directly which is a bit simpler:
var descriptors: Set<String> = []
for service in services {
let name = service.namespace.base.isEmpty ? service.name.base : "\(service.namespace.base).\(service.name.base)"
let (inserted, _) = descriptors.insert(name)
if !inserted {
throw ...
}
}
Motivation: The names of the namespaces/service/methods used in the type names in the generated code can be different from the names identifying the namespaces/service/methods in the IDL. This is why we need to have different properties for them. Methods also have a third name - for the function signatures. Modifications: - Created the Name struct which contains the name in three formats - Added the name properties in the ServiceDescriptor and MethodDescriptor from CodeGenerationRequest - Modified the existing tests accordingly - Added new validation methods verifying the generated names - Added tests for the new checks Result: Users will be able to set an identifying name, a generated upper case name and a generated upper case name for the namespaces, services and methods which are used in the generated code. --------- Co-authored-by: George Barnett <gbarnett@apple.com>
2 participants
Motivation:
The names of the namespaces/service/methods used in the type names from the generated code can be different from the names identifying the namespaces/service/methods in the IDL. Methods also have a third name - for the function signatures.
Modifications:
Result:
Users will be able to set an identifying name, a generated upper case and a generated lower case name for the namespaces, services and methods.