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: Add support for .none module type - Part 2: File Generators #2234

Conversation

calvincestari
Copy link
Member

@calvincestari calvincestari commented Apr 4, 2022

PR 2 for #2034 - This refactor grew larger than I anticipated so I'm trying to limit the changesets and keep the PRs focused; this PR refactors the file generators to support .none module type. This PR will merge into #2229, that PR will then pass CI, and they both then get merged into release/1.0.

  • All file generators now conform to the FileGenerator protocol making them simpler by providing just properties and sharing the rendering, file system code in the protocol extension.
  • FileGenerator handles resolving the path for the file, rendering the file into a data object and then writing that data to the resolved path in the file system.
  • PathResolver moved to FileGenerator.swift and renamed to FileTarget; the logic is only used in the shared file generator code.
  • The codegen configuration is now passed around as a reference wrapped struct to prevent copying it everywhere that needs configuration data. The downside is that they all get access to the full struct and not only their relevant parts of the configuration. This struct may change further when the codegen CLI is designed/built.

Note: This PR may fail the documentation checks of CI. That should be OK because main has not been merged into release/1.0 yet which means the latest changes to the documentation build workflow are not in this branch yet.

@@ -7,27 +8,28 @@ struct SchemaModuleFileGenerator {
/// - config: A configuration object specifying output behavior.
/// - fileManager: `FileManager` object used to create the file. Defaults to `FileManager.default`.
static func generate(
Copy link
Member Author

@calvincestari calvincestari Apr 4, 2022

Choose a reason for hiding this comment

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

This file generator is an anomaly because it doesn't conform to FileGenerator. The difficult part to making it conform is that .other doesn't create any output. It's easy to refactor this and make it conform for .swiftPackageManager and .none, but .other would require the protocol properties to become optional which I really don't want to do. I have not figured out a good way to resolve this yet. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

This is totally fine. No reason to do anything else here. Make the code do what it needs to do. The point of the protocol isn't to categorize file generators, it's to share functionality. If this object doesn't use the same shared functionality, then don't make it conform.

If at some point, we are passing an array of some FileGenerator around or something and need to include this, we can revisit that. But not something I think we should worry about today.

The other way to do this would be to make two protocols like this:

protocol FileGenerator {
 func generate(
    forConfig config: ReferenceWrapped<ApolloCodegenConfiguration>,
    fileManager: FileManager = FileManager.default
  ) throws
}

protocol TemplateGenerating: FileGenerator {
  var fileName: String { get }
  var template: TemplateRenderer { get }
  var target: FileTarget { get }
}


extension TemplateGenerating {
  func generate(
    forConfig config: ReferenceWrapped<ApolloCodegenConfiguration>,
    fileManager: FileManager = FileManager.default
  ) throws {
    let directoryPath = target.resolvePath(forConfig: config)
    let filePath = URL(fileURLWithPath: directoryPath).appendingPathComponent(fileName).path

    let rendered: String = template.render(forConfig: config)

    try fileManager.apollo.createFile(atPath: filePath, data: rendered.data(using: .utf8))
  }

This way, SchemaModuleFileGenerator could conform to FileGenerator without conforming to TemplateGenerating. This follows the I in the SOLID principles (interface segregation principle) really well. But this is really only relevant in the case that we need to pass these around abstractly as some FileGenerator, which is not currently necessary.

FYI:
I went with this naming because Swift API Design Guidelines recommend that protocol naming conventions follow:

  • Protocols that describe what something is should read as nouns (e.g. Collection).

  • Protocols that describe a capability should be named using the suffixes able, ible, or ing (e.g. Equatable, ProgressReporting).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I'll leave it as-is but great suggestion on the two protocols.

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.

This looks AMAZING!

Not important now, but wondering if ReferenceWrapped would be better as a property wrapper. Not sure if that is the right way to go, but now that we can use property wrappers in function arguments, could be cool.
https://www.swiftbysundell.com/tips/attaching-property-wrappers-to-function-arguments/

let rendered: String = template.render(forConfig: config)

try fileManager.apollo.createFile(atPath: filePath, data: rendered.data(using: .utf8))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

@calvincestari calvincestari merged commit 1607ece into 1.0/moduletype-none-filetemplates Apr 5, 2022
@calvincestari calvincestari deleted the 1.0/moduletype-none-filegenerators branch April 5, 2022 18:16
calvincestari added a commit that referenced this pull request Apr 5, 2022
…2234)

* Adapt EnumFileGenerator to FileGenerator
* Adapt ObjectFileGenerator to FileGenerator
* Adapt InterfaceFileGenerator to FileGenerator
* Adapt FragmentFileGenerator to FileGenerator
* Adapt InputObjectFileGenerator to FileGenerator
* Adapt OperationFileGenerator to FileGenerator
* Adapt SchemaFileGenerator to FileGenerator
* Adapt UnionFileGenerator to FileGenerator
* Add Equatable conformance to OperationDefinition
* Remove render function from TemplateRenderer protocol - only in extension
* Rename MockTemplate to MockFileTemplate
* Add FileGenerator protocol
* Move path resolver logic into FileGenerator
* Add FileGenerator tests
* Update SchemaModuleFileGenerator for ReferenceWrapped
* Adapt ApolloCodegen to ReferenceWrapped and FileGenerator
* fix: Test should match all rendered content
* fix: remove equality implementation, will be synthesized instead
calvincestari added a commit that referenced this pull request Apr 5, 2022
…2229)

* Add schema module template for module type .none
* Refactor SchemaModuleFileGenerator to use the new template and simplify logic
* Refactor to static enum case implementations for schema namespace
* First implementation of TemplateRenderer into EnumTemplate
* Add tests for basic implementation of TemplateRenderer
* Add TemplateRenderer tests
* Add target property to EnumTemplate
* Adapt FragmentTemplate to TemplateRenderer
* Adapt OperationDefinitionTemplate to TemplateRenderer
* Adapt InputObjectTemplate to TemplateRenderer
* Adapt InterfaceTemplate to TemplateRenderer
* Update EnumTemplate/FragmentTemplate/OperationDefinitionTemplate tests to reuse function to render subject
* Remove header and import renders from InterfaceTemplate template
* Adapt ObjectTemplate to TemplateRenderer
* Adapt SchemaTemplate to TemplateRendered
* Adapt SwiftPackageManagerModuleTemplate to TemplateRenderer
* Adapt UnionTemplate to TemplateRenderer
* Adapt SchemaModuleNamespaceTemplate to TemplateRenderer
* Refactor ImportStatementTemplate to serve TemplateString
* HeaderCommentTemplate can serve StaticString
* Refactor TemplateRenderer to support template targets
* Add documentation to templates
* refactor: Move Header/Import templates to TemplateRenderer as private

* feat: Add support for `.none` module type - Part 2: File Generators (#2234)
* Adapt EnumFileGenerator to FileGenerator
* Adapt ObjectFileGenerator to FileGenerator
* Adapt InterfaceFileGenerator to FileGenerator
* Adapt FragmentFileGenerator to FileGenerator
* Adapt InputObjectFileGenerator to FileGenerator
* Adapt OperationFileGenerator to FileGenerator
* Adapt SchemaFileGenerator to FileGenerator
* Adapt UnionFileGenerator to FileGenerator
* Add Equatable conformance to OperationDefinition
* Remove render function from TemplateRenderer protocol - only in extension
* Rename MockTemplate to MockFileTemplate
* Add FileGenerator protocol
* Move path resolver logic into FileGenerator
* Add FileGenerator tests
* Update SchemaModuleFileGenerator for ReferenceWrapped
* Adapt ApolloCodegen to ReferenceWrapped and FileGenerator
* fix: Test should match all rendered content
* fix: remove equality implementation, will be synthesized instead
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.

2 participants