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 codegen option about convert enum case name strategy #2478

Merged
merged 26 commits into from
Sep 8, 2022

Conversation

bannzai
Copy link
Contributor

@bannzai bannzai commented Sep 1, 2022

This pull request to release/1.0 branch. And related and depend on this PR: #2477

🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧

See #2477 before this PR.

Abstract

I use apollo-ios codegen version of v1-beta3. My service provide UPPER_CASE enum schema. It will genarate to case UPPER_CASE, but It was no good syntax in swift.
Therefore, I create about proposal PR of add to codegen option about enum case convert strategy. I would find a good form through this PR for generating enum case name.

Test

I confirmed to behavior with executing codegen script in my local. And Run UnitTests are passed.

@netlify
Copy link

netlify bot commented Sep 1, 2022

👷 Deploy request for apollo-ios-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit f37b28a

@bannzai bannzai changed the base branch from main to release/1.0 September 1, 2022 07:55
@netlify
Copy link

netlify bot commented Sep 1, 2022

👷 Deploy request for apollo-ios-docc pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit f37b28a

@AnthonyMDev
Copy link
Contributor

Thanks so much for the PRs! I like this approach a lot and we've actually discussed doing something similar. I think we may want to consider utilizing this case conversion strategy option for other things like fields and type names, not just enum cases.

We'll have a conversation about this next week and decide on a direction here.

@AnthonyMDev
Copy link
Contributor

We've decided that we are going to go ahead with this approach with a few minor changes! Thanks for making the PR @bannzai.

We want to change the naming from ConvertStrategy to ConversionStrategies.

Additionally, to support future use cases where we may add conversion strategies for other things, we want to refactor the configuration option to a struct that has a property for enumCases.

  public enum CaseConversionStrategy: String, Codable, Equatable {
    /// Default. Nothing different from the definition of a schema.
    case none
    /// Convert to lower camel case from `snake_case`, `UpperCamelCase`,`UPPERCASE`.
    case camelCase
  }

  public struct ConversionStrategies: Codable, Equatable {
    public let enumCases: CaseConversionStrategy = .camelCase
  }

I'm leaning towards defaulting to .camelCase instead of .none, here. @calvincestari what do you think?

@bannzai Do you want to make these changes to the PR so we can merge it in, or would you like me to do so and make a new PR?

@bannzai
Copy link
Contributor Author

bannzai commented Sep 7, 2022

@AnthonyMDev I will try it!

@calvincestari
Copy link
Member

I'm leaning towards defaulting to .camelCase instead of .none, here. @calvincestari what do you think?

.camelCase seems the most likely output that people would want in Swift code, I'm happy with that as the default.

@bannzai
Copy link
Contributor Author

bannzai commented Sep 8, 2022

I'm done to replace new strctures and testing it. Ready for review!

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

Thanks so much! This looks great!
Going to merge this in and then clean up the documentation a little bit.

@AnthonyMDev AnthonyMDev merged commit bb199f3 into apollographql:release/1.0 Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants