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

feat: Codegen config access modifier #2917

Merged
merged 31 commits into from
May 15, 2023

Conversation

calvincestari
Copy link
Member

@calvincestari calvincestari commented Mar 30, 2023

Closes #2630

Introduces an accessModifier associated value to ApolloCodegenConfiguration properties where the generated Swift code can have the access level specified by the user.

  • Built with defaults that match the generated code before this PR
  • Built with optional decoding so that users can use their existing JSON configuration files without error

@netlify
Copy link

netlify bot commented Mar 30, 2023

Deploy Preview for apollo-ios-docs canceled.

Name Link
🔨 Latest commit c14bdce
🔍 Latest deploy log https://app.netlify.com/sites/apollo-ios-docs/deploys/6462a5fb53144a0008a0b025

@@ -250,7 +255,7 @@ public struct ApolloCodegenConfiguration: Codable, Equatable {
/// - Note: Generated files must be manually added to your application target. The generated
/// schema types files will be namespaced with the value of your configuration's
/// `schemaNamespace` to prevent naming conflicts.
case embeddedInTarget(name: String)
case embeddedInTarget(name: String, accessModifier: AccessModifier = .internal)
Copy link
Member Author

Choose a reason for hiding this comment

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

It might seem odd that the default is .internal but if you look at the tests that have changed there were tests that named xyz_withPublicModifier and xyz_noPublicModifier. withPublicModifier was used for the tests related to a .swiftPackageManager module type and noPublicModifier was used for tests related to .embeddedInTarget module type. Since we weren't specifying any access modifier all the generated code would have implicitly had internal access level.

The caveat to this is the namespace enum, which we used to always generate as public - that was an error though and not aligned with the rest of the generated code for a .embeddedInTarget module type.

@@ -744,4 +746,57 @@ class ApolloCodegenConfigurationCodableTests: XCTestCase {
try JSONDecoder().decode(ApolloCodegenConfiguration.APQConfig.self, from: subject)
).to(throwError())
}

// MARK: - Optional Tests
Copy link
Member Author

@calvincestari calvincestari Mar 31, 2023

Choose a reason for hiding this comment

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

I would like to add more tests for optional properties of the JSON decoding - will do in a follow-up PR. Unrelated to this work.

@calvincestari calvincestari marked this pull request as ready for review March 31, 2023 00:23
@calvincestari calvincestari force-pushed the feature/codegen-access-modifier branch from e9e3a75 to 152236e Compare April 1, 2023 06:04
@calvincestari
Copy link
Member Author

Fixing the inner access control modifiers was brutal.

@calvincestari
Copy link
Member Author

Putting this back into draft because there is a regression re. #2302.

@calvincestari calvincestari marked this pull request as draft April 2, 2023 16:42
@calvincestari calvincestari force-pushed the feature/codegen-access-modifier branch from 539512e to b11ae45 Compare May 12, 2023 23:16
@calvincestari calvincestari marked this pull request as ready for review May 13, 2023 00:16
@calvincestari calvincestari requested a review from BobaFetters May 13, 2023 01:58
This was referenced May 15, 2023
@calvincestari calvincestari merged commit c4600ff into main May 15, 2023
@calvincestari calvincestari deleted the feature/codegen-access-modifier branch May 15, 2023 21:57
@lhunath
Copy link

lhunath commented Jun 8, 2023

Please see #3066 for a regression likely related to this change.

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.

Add option to generate objects with internal access modifier
4 participants