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: Schema/Target/Module name with spaces breaks generated code #2760

Merged
merged 8 commits into from
Jan 9, 2023

Conversation

calvincestari
Copy link
Member

Fixes #2653
Related to #2664

This PR refactors the flow of configuration validation logic.

  • Validation logic that is not specific to only the CLI command line parameters is now all in ApolloCodegen.
  • There is a public function _validation that performs config-only validation, not dependent on the schema nor operations, that can be called from the CLI.
  • There is also a fix to the install CLI plugin where it would have failed if the path had spaces in it.

@netlify
Copy link

netlify bot commented Jan 9, 2023

Deploy Preview for apollo-ios-docs canceled.

Name Link
🔨 Latest commit a492d54
🔍 Latest deploy log https://app.netlify.com/sites/apollo-ios-docs/deploys/63bc92daf70c5f000833dc83

/// is only a static analysis of the configuration.
///
/// - Parameter config: Code generation configuration settings.
public static func _validate(config: ApolloCodegenConfiguration) throws {
Copy link
Member Author

Choose a reason for hiding this comment

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

The public method prefixed with _ to indicate that while it's public it is still considered private.

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.

Minor suggestion, but looks good to me!

Sources/CodegenCLI/Commands/Initialize.swift Show resolved Hide resolved
@@ -76,14 +79,14 @@ public class ApolloCodegen {
rootURL: rootURL
)

try validate(config: configContext)
try validate(context: configContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

These two methods are validating different things I imagine, but it's unclear what those are (other than one uses the compilation result). Can we rename them to indicate what's going on here?

My assumption is that one is validating the context, the other is validating the compilationResult, and just needs info from the context.

If that's the case, I'd suggest
validate(_ context: ConfigurationContext)
and
validate(_ compilationResult: CompilationResult, with context: ConfigurationContext)

Copy link
Member Author

Choose a reason for hiding this comment

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

How about this? - a492d54

  • Takes the suggestion of the nameless parameter
  • Keeps compilationResult as a second param but using the label with because it's still validation of the configuration but dependent on the compilation results rather than validating the compilation results.

@calvincestari calvincestari merged commit daf1250 into main Jan 9, 2023
@calvincestari calvincestari deleted the fix/schema-target-module-name-with-spaces branch January 9, 2023 22:35
@calvincestari calvincestari mentioned this pull request Jan 10, 2023
7 tasks
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.

Schema/Target/Module name with spaces in it breaks generated code
2 participants