update: CLI defaults #2673

calvincestari · 2022-11-16T23:35:37Z

Updates the swift-argument-parser dependency to the latest version, 1.2.0 (both SPM and Xcode)
Changes output.operations default to .inSchemaModule
Requires the user to specify --module-type on CLI init command
Requires the user to specify --target-name when the module type is .embeddedInTarget
Updates all documentation referencing the init command to detail the new parameters

netlify · 2022-11-16T23:35:43Z

✅ Deploy Preview for apollo-ios-docs ready!

Name	Link
🔨 Latest commit	`7a73efd`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-ios-docs/deploys/63769459ed87680008348f8a
😎 Deploy Preview	https://deploy-preview-2673--apollo-ios-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Sources/CodegenCLI/Commands/Initialize.swift

Sources/ApolloCodegenLib/ApolloCodegenConfiguration.swift

Sources/CodegenCLI/Commands/Initialize.swift

Tests/ApolloCodegenTests/CodeGeneration/Templates/InputObjectTemplateTests.swift

Tests/CodegenCLITests/Commands/InitializeTests.swift

AnthonyMDev

Looks really good, just a couple comments around tests and documentation mostly!

I want to do a more comprehensive review of the docs because this only shows the files you did change, and I think there should be some additions to docs in the getting started guide (and maybe other areas?) to really explain that you need to pick a module type and link to the section of the docs that explains what each of those options really does.

I think the docs need to clearly explain that this is a decision that affects your project structure, and depending on what you select here, how you include your generated files is going to be significantly different.

AnthonyMDev · 2022-11-17T00:10:54Z

docs/shared/pods-setup-codegen-panel.mdx

 ```

-`${MySchemaName}` should be the name you want for the namespace of your generated schema files.
+* `${MySchemaName}` should be the name you want for the namespace of your generated schema files.
+* `other` is the module type to use when your dependency manager for Apollo iOS is CocoaPods.


I think I have a problem with this. Let's talk about it more.

I don't think that telling them it's a no-brainer and just to use other if they are using Cocoapods is accurate.

other is used if you want to package your generated schema as a separate module and distribute that as a Pod. But what we've found is that most users actually just want to embed the generated models right into their application target. They are just including Apollo via Cocoapods.

Even though we don't believe that is the best choice, it does seem to be the default expectation of users. So I think embeddedInTarget is still the default module type people would want to use. Either way, we should be very clear in explaining to people that this is a choice to be made and they should understand what the implications of each possible choice is. That way we aren't making embeddedInTarget the path of least resistance, but we also aren't confusing people who assumed that it would work that way, when their generated files don't link to their project and build properly.

This is in a CocoaPods focused panel, which is why it says to use other - it's contextual.

If they added Apollo to their project via CocoaPods the CLI is built with the COCOAPODS flag and will therefore always add cocoapodsCompatibleImportStatements=true to the initialization file. This happens without them knowing and if they used anything other than other the generated code wouldn't build because the import statements would be incorrect.

There is nuance and edge cases but the overwhelming majority of users that use CocoaPods should be using other as the module type here.

AnthonyMDev · 2022-11-17T00:11:00Z

docs/shared/spm-setup-codegen-panel.mdx

 ```

-`${MySchemaName}` should be the name you want for the namespace of your generated schema files.
+* `${MySchemaName}` should be the name you want for the namespace of your generated schema files.
+* `swiftPackageManager` is the module type to use when your dependency manager for Apollo iOS is Swift Package Manager.


Same as above

This is in an SPM focused panel, also contextual.

AnthonyMDev · 2022-11-17T00:12:26Z

Tests/CodegenCLITests/Commands/InitializeTests.swift

@@ -71,11 +94,69 @@ class InitializeTests: XCTestCase {
    expect(command.schemaName).to(equal("ShortFormatSchemaName"))
  }

+  func test__parsing__givenParameters_moduleNameLongFormat_shouldParse() throws {


While these tests are awesome and super comprehensive, remind me to have a conversation with you about my views on testing behavior vs configuration! Would love to save you some time in the future possibly.

AnthonyMDev · 2022-11-17T00:15:50Z

Sources/CodegenCLI/Commands/Initialize.swift

-            \(supportCocoaPods ? "\"other\"" : "\"swiftPackageManager\"") : {
-            }
+            "\(moduleType)" : {
+            \(moduleTarget)


I could be wrong, but this looks like the indentation is off, and that it may be missing a }? Not sure. Do we have unit tests for this? (I have another comment in the InitializeTests asking about testing this as well.)

The JSON is correct, otherwise none of the decoding from the return string of minimalJSON into an ApolloCodegenConfiguration instance would succeed.

The indentation is also correct, it looks odd because of where \(moduleTarget) is on line 221. If that was indented two more to 'look' correct, then 194 would need to move two backwards.

calvincestari · 2022-11-17T06:46:10Z

I think there should be some additions to docs in the getting started guide (and maybe other areas?) to really explain that you need to pick a module type and link to the section of the docs that explains what each of those options really does.

I think the docs need to clearly explain that this is a decision that affects your project structure, and depending on what you select here, how you include your generated files is going to be significantly different.

Yes, I agree with this. More thorough explanation of the modules types will save us a bunch of issues being created and help users to be less confused about the output they get.

calvincestari added 6 commits November 16, 2022 12:09

Changes operations output config default

34c101c

Bumps swift-argument-parser dependency to minimum 1.2.0

4525c8e

Adds --module-type and --target-name to init required parameters

12b3774

Adds tests to ensure enum compatibility

3bc12aa

Fix CLI integration test

aa3f757

Updates documentation

ffba679

calvincestari requested a review from AnthonyMDev November 16, 2022 23:35

calvincestari commented Nov 16, 2022

View reviewed changes

Sources/CodegenCLI/Commands/Initialize.swift Show resolved Hide resolved

calvincestari commented Nov 16, 2022

View reviewed changes

Sources/CodegenCLI/Commands/Initialize.swift Show resolved Hide resolved

AnthonyMDev reviewed Nov 17, 2022

View reviewed changes

AnthonyMDev requested changes Nov 17, 2022

View reviewed changes

calvincestari added 2 commits November 16, 2022 21:40

Update header doc for new default

fbcddb7

Refactor validation error

def919a

calvincestari added 2 commits November 17, 2022 12:06

Remove extraneous tests

82717b2

Update documentation for embeddedInTarget module type notes

7a73efd

AnthonyMDev approved these changes Nov 17, 2022

View reviewed changes

calvincestari merged commit 5b2336c into main Nov 17, 2022

calvincestari deleted the update/codegen-cli-defaults branch November 17, 2022 21:38

calvincestari mentioned this pull request Nov 18, 2022

release: Prepare 1.0.4 #2676

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update: CLI defaults #2673

update: CLI defaults #2673

calvincestari commented Nov 16, 2022

netlify bot commented Nov 16, 2022 •

edited

Loading

AnthonyMDev left a comment

AnthonyMDev Nov 17, 2022

calvincestari Nov 17, 2022

AnthonyMDev Nov 17, 2022

calvincestari Nov 17, 2022

AnthonyMDev Nov 17, 2022

AnthonyMDev Nov 17, 2022

calvincestari Nov 17, 2022 •

edited

Loading

calvincestari commented Nov 17, 2022

update: CLI defaults #2673

update: CLI defaults #2673

Conversation

calvincestari commented Nov 16, 2022

netlify bot commented Nov 16, 2022 • edited Loading

✅ Deploy Preview for apollo-ios-docs ready!

AnthonyMDev left a comment

Choose a reason for hiding this comment

AnthonyMDev Nov 17, 2022

Choose a reason for hiding this comment

calvincestari Nov 17, 2022

Choose a reason for hiding this comment

AnthonyMDev Nov 17, 2022

Choose a reason for hiding this comment

calvincestari Nov 17, 2022

Choose a reason for hiding this comment

AnthonyMDev Nov 17, 2022

Choose a reason for hiding this comment

AnthonyMDev Nov 17, 2022

Choose a reason for hiding this comment

calvincestari Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

calvincestari commented Nov 17, 2022

netlify bot commented Nov 16, 2022 •

edited

Loading

calvincestari Nov 17, 2022 •

edited

Loading