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

Add code gen config for the module name #2201

Merged
merged 3 commits into from
Feb 28, 2025

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Feb 26, 2025

Modifications:

  • Add config to the code generator allowing a different module name to be specified.
  • Remove unused code

Result:

The grpc module name can be specified in generated code

Modifications:

- Add config to the code generator allowing a different module name to
  be specified.
- Remove unused code

Result:

The grpc module name can be specified in generated code
@glbrntt glbrntt requested a review from gjcairo February 26, 2025 08:29
@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Feb 26, 2025
@glbrntt
Copy link
Collaborator Author

glbrntt commented Feb 26, 2025

The API breakage checker is a false positive, this is a package level API:

1 breaking change detected in GRPCCodeGen:
  💔 API breakage: func IDLToStructuredSwiftTranslator.makeImports(dependencies:accessLevel:accessLevelOnImports:) has been renamed to func makeImports(dependencies:accessLevel:accessLevelOnImports:grpcCoreModuleName:)

@glbrntt
Copy link
Collaborator Author

glbrntt commented Feb 26, 2025

The examples are failing on nightly-main and nightly-next because of grpc/grpc-swift-nio-transport#69 (which is yet to be released).

@gjcairo
Copy link
Collaborator

gjcairo commented Feb 26, 2025

Why do we need this exactly? Just for testing? If so, can we not make the new grpcCoreModuleName property package instead of public?

@glbrntt glbrntt marked this pull request as draft February 26, 2025 11:22
@glbrntt glbrntt marked this pull request as ready for review February 26, 2025 15:30
Copy link
Collaborator

@gjcairo gjcairo left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit

return self.grpcCore + "." + type
}

func serverRequest(forType type: String?, streaming: Bool) -> ExistingTypeDescription {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think having isStreaming in all these funcs instead of streaming makes it more consistent with requestResponse (and it's also more idiomatic).

Suggested change
func serverRequest(forType type: String?, streaming: Bool) -> ExistingTypeDescription {
func serverRequest(forType type: String?, isStreaming: Bool) -> ExistingTypeDescription {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 8fa87bf

@glbrntt glbrntt enabled auto-merge (squash) February 28, 2025 13:28
@glbrntt glbrntt disabled auto-merge February 28, 2025 13:43
@glbrntt glbrntt merged commit f9fbd68 into grpc:main Feb 28, 2025
27 of 30 checks passed
@glbrntt glbrntt deleted the code-gen-module-name branch February 28, 2025 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants