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

Fix casing of generated services and methods #8

Merged
merged 6 commits into from
Oct 4, 2024

Conversation

clintonpi
Copy link
Contributor

Motivation:

Given a method name, say ImportCSV, from a .proto file, the generated method name in upper and lower camel case is ImportCsv and importCsv respectively. The generated method name (which is already expected to be in upper camel case) in upper and lower camel case should be ImportCSV and importCSV respectively.

Modifications:

  • Replace the method used for generating service and method names in lower camel case with a newly implemented method.
  • Do not convert the base names of services and methods to upper camel case as they are expected to already be in upper camel case.

Result:

The casing of service and method names will be preserved when generating stubs.

Motivation:

Given a method name, say `ImportCSV`, from a `.proto` file, the generated method name in upper and lower camel case is `ImportCsv` and `importCsv` respectively. The generated method name (which is already expected to be in upper camel case) in upper and lower camel case should be `ImportCSV` and `importCSV` respectively.

Modifications:

- Replace the method used for generating service and method names in lower camel case with a newly implemented method.
- Do not convert the base names of services and methods to upper camel case as they are expected to already be in upper camel case.

Result:

The casing of service and method names will be preserved when generating stubs.
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Broadly looks good. I left a few comments inline about simplifying a couple of bits.

package struct CamelCaser {
/// Converts a string from upper camel case to lower camel case.
package static func toLowerCamelCase(_ s: String) -> String {
if s.isEmpty { return "" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this check. If the string is empty then indexOfFirstLowerCase will be nil and we'll return s.lowercased() (which is fine)


package struct CamelCaser {
/// Converts a string from upper camel case to lower camel case.
package static func toLowerCamelCase(_ s: String) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Single character names are rarely a good idea. input is fine here.

package static func toLowerCamelCase(_ s: String) -> String {
if s.isEmpty { return "" }

let indexOfFirstLowerCase = s.firstIndex(where: { $0 != "_" && $0.lowercased() == String($0) })
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the "_" is relevant here. We also don't need to convert the character into a string, we can just do:

Suggested change
let indexOfFirstLowerCase = s.firstIndex(where: { $0 != "_" && $0.lowercased() == String($0) })
let indexOfFirstLowerCase = s.firstIndex(where: { $0.isLowercase })

package static func toLowerCamelCase(_ s: String) -> String {
if s.isEmpty { return "" }

let indexOfFirstLowerCase = s.firstIndex(where: { $0 != "_" && $0.lowercased() == String($0) })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also the correct casing of indexOfFirstLowerCase is indexOfFirstLowercase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ironic 😂

Comment on lines 41 to 44
} else {
// `s` did not contain any lower case letter.
return s.lowercased()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can do an early exit if indexOfFirstLowercase is nil:

guard let indexOfFirstLowerCase = s.firstIndex(where: { ... }) else {
  return s.lowercased()
}

@@ -15,7 +15,6 @@
*/

internal import Foundation
internal import SwiftProtobuf
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this import not used for anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was imported to access NamingUtils which is no longer used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

NamingUtils comes from the SwiftProtobufPluginLibrary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes. That means SwiftProtobuf was already not in use.

@clintonpi clintonpi requested a review from glbrntt October 4, 2024 15:59
@@ -15,7 +15,6 @@
*/

internal import Foundation
internal import SwiftProtobuf
Copy link
Collaborator

Choose a reason for hiding this comment

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

NamingUtils comes from the SwiftProtobufPluginLibrary

* limitations under the License.
*/

package struct CamelCaser {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just being used as a namespace so should be an enum:

Suggested change
package struct CamelCaser {
package enum CamelCaser {

@clintonpi clintonpi requested a review from glbrntt October 4, 2024 16:46
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Great, thank you @clintonpi

@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Oct 4, 2024
@glbrntt glbrntt merged commit 658814e into grpc:main Oct 4, 2024
3 of 4 checks passed
@clintonpi clintonpi deleted the fix-casing-of-generated-names branch October 4, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants