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 Files Resource Synthesizer #3584

Merged
merged 3 commits into from
Oct 22, 2021
Merged

Conversation

mollyIV
Copy link
Collaborator

@mollyIV mollyIV commented Oct 21, 2021

Resolves #3583

Short description πŸ“

This pull request is adding the support for Files Parser to Resource Synthesizer.

.files(extensions: ["html"])

Example of code generated by the bundled template:

public enum Files {
  public enum FrameworkA {
    public enum Resources {
      public static let exampleHtml = File(name: "example", ext: "html", relativePath: "", mimeType: "text/html")
    }
  }
}

I added a few comments to highlight the most important changes and some to ask questions about the implementation πŸ™

Checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

@@ -64,7 +64,7 @@ let package = Package(
.package(url: "https://github.com/apple/swift-argument-parser.git", .upToNextMajor(from: "0.4.3")),
.package(url: "https://github.com/maparoni/Zip.git", .revision("059e7346082d02de16220cd79df7db18ddeba8c3")),
.package(url: "https://github.com/tuist/GraphViz.git", .branch("tuist")),
.package(url: "https://github.com/fortmarek/SwiftGen", .branch("stable")),
.package(url: "https://github.com/SwiftGen/SwiftGen", .upToNextMajor(from: "6.5.1")),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switched to SwiftGen repository instead of a fork. The reason behind using a fork was that PathKit used to be below 1.0.0 in the main repo that has changed.

@@ -0,0 +1,117 @@
// swiftlint:disable line_length
extension SynthesizedResourceInterfaceTemplates {
static let filesTemplate = """
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@mollyIV would you mind adding that link in a documentation comment?

Comment on lines +82 to +83
let file = name + (ext.flatMap { "." + $0 } ?? "")
fatalError("Could not locate file named" + file)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two lines have been modified slightly in order to make the compiler happy:

- let file = name + (ext.flatMap { ".\($0)" } ?? "")
- fatalError("Could not locate file named \(file)")
+ let file = name + (ext.flatMap { "." + $0 } ?? "")
+ fatalError("Could not locate file named" + file)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea why the other code was not working? πŸ€”

Copy link
Collaborator Author

@mollyIV mollyIV Oct 22, 2021

Choose a reason for hiding this comment

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

Basically, the compiler is not happy with it when the content is wrapper into """ """:

let string = """
  let name = "a name"
  print("Hello \(name)" // <-- The compiler will complain saying "Cannot find 'name' in scope"
"""

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you just need to escape the \ in this case?

let string = """
  let name = "a name"
  print("Hello \\(name)" // <-- The compiler will complain saying "Cannot find 'name' in scope"
"""

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's another way of solving the same problem πŸ‘

@mollyIV
Copy link
Collaborator Author

mollyIV commented Oct 21, 2021

Unfortunately didn't find existing unit tests for parsers, neither acceptance tests. What's the recommended strategy of parsers / resource synthesizer testing then? πŸ€”

I performed a manual testing and the files parser is working as expected.

ios_app_with_resources.zip

To test the feature:

  1. Download an attached demo project.
  2. Generate a project via tuist generate.
  3. Open FrameworkA/FrameworkA.xcodeproj.
  4. Check if a project compile and Files+FrameworkA.swift content.

Accessing the files:

let exampleHTML = Files.FrameworkA.Resources.exampleHtml
let exampleTXT = Files.FrameworkA.Resources.exampleTxt

@fortmarek fortmarek self-requested a review October 21, 2021 08:46
Package.resolved Show resolved Hide resolved
Comment on lines +82 to +83
let file = name + (ext.flatMap { "." + $0 } ?? "")
fatalError("Could not locate file named" + file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea why the other code was not working? πŸ€”

CHANGELOG.md Outdated Show resolved Hide resolved
@ezraberch
Copy link
Contributor

Unfortunately didn't find existing unit tests for parsers, neither acceptance tests. What's the recommended strategy of parsers / resource synthesizer testing then? πŸ€”

I've created an acceptance test based on your sample project here: 9af9b0d. This will verify that the project generates and builds. It will also run the project's unit tests, so just replace the placeholder in AppTests.swift with whatever you want.

@mollyIV mollyIV marked this pull request as ready for review October 21, 2021 21:08
@danieleformichelli danieleformichelli changed the title Add Add Files Resource Synthesizer Add Files Resource Synthesizer Oct 22, 2021
@pepicrft pepicrft merged commit 277f1aa into main Oct 22, 2021
@pepicrft pepicrft deleted the add-files-resource-synthesizer branch October 22, 2021 16:48
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 Files Resource Synthesizer
4 participants