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

Added codegen support for source file #3448

Merged
merged 17 commits into from
Sep 20, 2021

Conversation

pavm035
Copy link
Contributor

@pavm035 pavm035 commented Sep 14, 2021

Added Filecodegen to support source file attributes in settings

Since we have few special type source files like intentdefinition and mlmodels that supports adding codegen attributes in settings

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.

@kwridan kwridan self-assigned this Sep 15, 2021
@kwridan kwridan self-requested a review September 15, 2021 05:36
Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this @pavm035 !

Tested locally and works great 👍

Left some minor comments, but in general it's in good shape!

Comment on lines 1 to 6
//
// FileCodeGen.swift
// FileCodeGen
//
// Created by Mahadevaiah, Pavan | Pavan | ECMPD on 2021/09/14.
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: Worth removing this for consistency with the rest of the codebase where all header comments are omitted

@@ -168,7 +168,8 @@ extension TuistGraph.Target {
let allSources = try TuistGraph.Target.sources(targetName: targetName, sources: manifest.sources?.globs.map { glob in
let globPath = try generatorPaths.resolve(path: glob.glob).pathString
let excluding: [String] = try glob.excluding.compactMap { try generatorPaths.resolve(path: $0).pathString }
return TuistGraph.SourceFileGlob(glob: globPath, excluding: excluding, compilerFlags: glob.compilerFlags)
let mappedCodeGen = TuistGraph.FileCodeGen.from(manifest: glob.codeGen)
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: an alternate form that can mitigate the need to deal with optionals in the from method would be

let mappedCodeGen = glob.codeGen.map(TuistGraph.FileCodeGen.from)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you please bit more detailed code the above snippet returns FileCodeGen??

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah sorry, meant that would help make the from method be non optional:

e.g.

static func from(manifest: ProjectDescription.FileCodeGen) -> TuistGraph.FileCodeGen {
  // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -14,7 +14,13 @@ final class TargetTests: XCTestCase {
bundleId: "bundle_id",
deploymentTarget: .iOS(targetVersion: "13.1", devices: [.iphone, .ipad]),
infoPlist: "info.plist",
sources: "sources/*",
sources: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

this particular test intentionally had sources: "sources/*" as it was verifying sources is able to accept string literals (for backward compatibility).

For this particular PR, updating test_toJSON_withFileList should suffice

projects/docs/docs/manifests/project.md Show resolved Hide resolved
Comment on lines 19 to 21
let model = MyModel()
model.save()
model.load()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this compile?

For this fixture, it would be handy to import Intents and used one of the codegened intent types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it compiles actually i copied this folder, but let me update this code to use intent

/// - `project`: project codegen attribute `settings = {ATTRIBUTES = (project_codegen, )}`
/// - `disabled`: disabled codegen attribute `settings = {ATTRIBUTES = (no_codegen, )}`
///
public enum FileCodeGen: Codable, Equatable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To support Xcode 12.5.1 (which doesn't yet support enum codable synthesis), we'll need to make this enum have a string raw value.

public enum FileCodeGen: String, Codable, Equatable

/// - `project`: project codegen attribute `settings = {ATTRIBUTES = (project_codegen, )}`
/// - `disabled`: disabled codegen attribute `settings = {ATTRIBUTES = (no_codegen, )}`
///
public enum FileCodeGen: Codable, Equatable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here (needs a string raw value)

@kwridan kwridan changed the title - Added codegen support for source file Added codegen support for source file Sep 15, 2021
Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the updates @pavm035

@danieleformichelli
Copy link
Collaborator

Hey @pavm035, could you please align your branch with main so that CI can pass? 🙏

@pavm035
Copy link
Contributor Author

pavm035 commented Sep 17, 2021

@danyf90 Done can you please confirm

pavm035 and others added 5 commits September 17, 2021 18:59
Co-authored-by: Daniele Formichelli <df@bendingspoons.com>
Co-authored-by: Daniele Formichelli <df@bendingspoons.com>
Co-authored-by: Daniele Formichelli <df@bendingspoons.com>
Co-authored-by: Daniele Formichelli <df@bendingspoons.com>
Package.resolved Outdated Show resolved Hide resolved
@danieleformichelli danieleformichelli merged commit e859f66 into tuist:main Sep 20, 2021
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.

3 participants