[CodeGenLib] Translating dependencies into StructuredSwiftRepresentation #1752
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: Dependency representations within CodeGeneratorRequest and StructuredSwiftRepresentation don't match (each contains fields that the other one doesn't have). The CodeGenerationRequest representation of a dependency doesn't contain preconcurrency requirements and spi, while the StructuredSwiftRepresentation doesn't allow items imports (func, class etc.). We need to add the missing fields to both representation in order to translate the imports, without losing information. Modifications: - Added the preconcurrency ans spi parameters to the CodeGenerationRequest Dependency struct (and the PreconcurrencyRequirement struct to represent that encapsulates the possible requirement types) - Added the item property to the StructuredSwiftRepresentation ImportDescription struct (and the Item struct backed up by an item representing the possible cases). - Created the function that translates a CodeGenerationRequest.Dependency into a StructuredSwiftRepresentation.ImportDescription. - Added the item rendering in the TextBasedRenderer. - Added tests for the TextRenderer and the IDLToStructuredSwiftRepresentationTranslator that check the imports. Result: Imports can now be translated and added in the generated code.
gjcairo
requested changes
Jan 3, 2024
Collaborator
gjcairo
left a comment
gjcairo
left a comment
There was a problem hiding this comment.
Looking good, left only a few small comments.
Comment on lines
203
to
205
| case always | ||
| case never | ||
| case onOS([String]) |
Collaborator
There was a problem hiding this comment.
Nit: I wonder if required, notRequired and requiredOnOS([String]) would be better names here?
Comment on lines
27
to
29
| let imports = | ||
| try [ImportDescription(moduleName: "GRPCCore")] | ||
| + codeGenerationRequest.dependencies.map { try self.translateImport(dependency: $0) } |
Collaborator
There was a problem hiding this comment.
Nit: you can replace this map+concatenation with a reduce(into:):
Suggested change
| let imports = | |
| try [ImportDescription(moduleName: "GRPCCore")] | |
| + codeGenerationRequest.dependencies.map { try self.translateImport(dependency: $0) } | |
| let imports = try codeGenerationRequest.dependencies.reduce( | |
| into: [ImportDescription(moduleName: "GRPCCore")] | |
| ) { partialResult, newDependency in | |
| try partialResult.append(translateImport(dependency: newDependency)) | |
| } |
Comment on lines
58
to
60
| if let matchedKind = ImportDescription.Kind.allCases.first(where: { | ||
| "\($0)" == item.kind.value.description | ||
| }) { |
Collaborator
There was a problem hiding this comment.
ImportDescription.Kind has a raw value of String. This means that it provides an init?(rawValue:) initialiser. You can take advantage of it to avoid iterating over all cases as you're doing here - if the provided raw value doesn't match an enum case, the initialiser will return nil:
Suggested change
| if let matchedKind = ImportDescription.Kind.allCases.first(where: { | |
| "\($0)" == item.kind.value.description | |
| }) { | |
| if let matchedKind = ImportDescription.Kind(rawValue: item.kind.value.description) { |
| public struct Kind { | ||
| /// Describes the keyword associated with the imported item. | ||
| internal enum Value { | ||
| internal enum Value: CustomStringConvertible { |
Collaborator
There was a problem hiding this comment.
A bit of a nit, but ImportDescription.Kind, which basically mirrors this type, has a String raw value instead of conforming to CustomStringConvertible. Is there a reason why this enum conforms to CustomStringConvertible instead of doing the same as ImportDescription.Kind?
Collaborator
Author
There was a problem hiding this comment.
Not really, I will use raw values instead in both cases
Comment on lines
73
to
74
| let preconcurrency = dependency.preconcurrency.value | ||
| switch preconcurrency { |
Collaborator
There was a problem hiding this comment.
Nit, but as you're not using preconcurrency anywhere else, you can switch directly over the value without declaring the let before:
Suggested change
| let preconcurrency = dependency.preconcurrency.value | |
| switch preconcurrency { | |
| switch dependency.preconcurrency.value { |
gjcairo
approved these changes
Jan 4, 2024
glbrntt
requested changes
Jan 8, 2024
glbrntt
approved these changes
Jan 12, 2024
glbrntt
pushed a commit
to glbrntt/grpc-swift
that referenced
this pull request
Feb 5, 2024
…ion (grpc#1752) Translating dependencies into StructuredSwiftRepresentation Motivation: Dependency representations within CodeGeneratorRequest and StructuredSwiftRepresentation don't match (each contains fields that the other one doesn't have). The CodeGenerationRequest representation of a dependency doesn't contain preconcurrency requirements and spi, while the StructuredSwiftRepresentation doesn't allow items imports (func, class etc.). We need to add the missing fields to both representation in order to translate the imports, without losing information. Modifications: - Added the preconcurrency ans spi parameters to the CodeGenerationRequest Dependency struct (and the PreconcurrencyRequirement struct to represent that encapsulates the possible requirement types) - Added the item property to the StructuredSwiftRepresentation ImportDescription struct (and the Item struct backed up by an item representing the possible cases). - Created the function that translates a CodeGenerationRequest.Dependency into a StructuredSwiftRepresentation.ImportDescription. - Added the item rendering in the TextBasedRenderer. - Added tests for the TextRenderer and the IDLToStructuredSwiftRepresentationTranslator that check the imports. Result: Imports can now be translated and added in the generated code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Dependency representations within CodeGeneratorRequest (Dependency) and StructuredSwiftRepresentation (ImportDescription) don't match, as each contains fields that the other one doesn't have. The CodeGenerationRequest representation of a dependency doesn't contain the preconcurrency requirement and spi, while the StructuredSwiftRepresentation doesn't allow importing items (func, class etc.). We need to add the missing fields to both representations in order to translate the imports, without losing information.
Modifications:
Result:
Imports can now be translated and added in the generated code.