Make empty generated source files descriptive #2151
Merged
Conversation
This file contains 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: Sometimes protobuf definition files contain no gRPC services and result in an empty file. This is intentional but could be confusing. Modifications: Empty files now contain a comment indicating they are intentional. ``` // This file contained no services. ``` Result: More descriptive empty source files.
glbrntt
requested changes
Jan 6, 2025
Comment on lines
98
to
103
| if structuredSwiftRepresentation.file.contents.codeBlocks.isEmpty { | ||
| structuredSwiftRepresentation.file.contents.imports = [] | ||
| structuredSwiftRepresentation.file.contents.codeBlocks.append( | ||
| CodeBlock(comment: .inline("This file contained no services.")) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Can you do this within the translator? And could you also add a test for this please?
glbrntt
approved these changes
Jan 6, 2025
| topComment: .preFormatted(codeGenerationRequest.leadingTrivia), | ||
| imports: try self.makeImports( | ||
| let imports: [ImportDescription] | ||
| if codeBlocks.isEmpty { |
There was a problem hiding this comment.
Can we check this instead? It's easy to imagine unconditionally adding a code block which would result in this check failing.
Suggested change
| if codeBlocks.isEmpty { | |
| if codeGenerationRequest.services.isEmpty { |
rnro
force-pushed
the
make_empty_files_more_descriptive
branch
from
c270085 to
ec55cd5
Compare
|
I have switched the import tests to a new set of tests which render the imports directly. This is because this code change meant that the minimal tests which defined no services no longer rendered the imports. |
glbrntt
reviewed
Jan 6, 2025
| @@ -87,7 +97,7 @@ struct IDLToStructuredSwiftTranslator: Translator { | |||
| return StructuredSwiftRepresentation(file: file) | |||
| } | |||
|
|
|||
| private func makeImports( | |||
| internal func makeImports( | |||
There was a problem hiding this comment.
package is your friend here
| @@ -0,0 +1,200 @@ | |||
| /* | |||
| * Copyright 2024, gRPC Authors All rights reserved. | |||
There was a problem hiding this comment.
Suggested change
| * Copyright 2024, gRPC Authors All rights reserved. | |
| * Copyright 2025, gRPC Authors All rights reserved. |
|
|
||
| import Testing | ||
|
|
||
| @testable import GRPCCodeGen |
There was a problem hiding this comment.
We can use a regular import if you use package appropriately
glbrntt
approved these changes
Jan 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
Sometimes protobuf definition files contain no gRPC services and result in an empty file. This is intentional but could be confusing.
Modifications:
Empty files now contain a comment indicating they are intentional.
This is analogous to the behavior of swift-protobuf which adds:
Result:
More descriptive empty source files.