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 localized *.intentdefinition have to be added to build source phases #720

Merged
merged 10 commits into from
Nov 19, 2019

Conversation

giginet
Copy link
Collaborator

@giginet giginet commented Nov 18, 2019

localised .intentdefinition to sources does not work · Issue #614 · yonaskolb/XcodeGen

closes #614

Problem

In the situation, the project has localized *.intentdefinition, they have to be added to Copy Bundle Resources phase instead of Compile Sources phase.

IntentDefinition
├── Base.lproj
│   └── Intents.intentdefinition
├── Info.plist
├── en.lproj
│   └── Intents.strings
└── ja.lproj
    └── Intents.strings

Screen Shot 2019-11-19 at 3 17 45

Description

Localized *.intentdefinition should be Sources. However, the current implementation treats as Resources.

This PR makes the issue to fix.

Before After

@giginet giginet requested a review from yonaskolb November 18, 2019 18:29
@giginet giginet self-assigned this Nov 18, 2019
@giginet giginet changed the title Fix localized *intentdefinition should be added to build source phase Fix localized *intentdefinition have to be added to build source phases Nov 18, 2019
path: filePath,
fileReference: variantGroup,
buildFile: PBXBuildFile(file: variantGroup),
buildPhase: .resources
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previous implementation, all files under *.lproj are treated as Resources. However, they should be generated by generateSourceFile function.

@giginet giginet changed the title Fix localized *intentdefinition have to be added to build source phases Fix localized *.intentdefinition have to be added to build source phases Nov 18, 2019
@keith
Copy link
Collaborator

keith commented Nov 18, 2019

Note that depending on what type of intents you're setting up you may prefer this to be in the resources phase and not the compile sources phase because you may not want generated code from this.

@giginet
Copy link
Collaborator Author

giginet commented Nov 18, 2019

If you want to treat them as Resources, you can specify buildPhase

sources:
   - path: Sources/Intents.intentdefinition
     buildPhase: resources

Copy link
Collaborator

@brentleyjones brentleyjones left a comment

Choose a reason for hiding this comment

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

Looks correct to me

@giginet
Copy link
Collaborator Author

giginet commented Nov 19, 2019

Thanks!

@giginet giginet merged commit 538d1c6 into yonaskolb:master Nov 19, 2019
@giginet giginet deleted the intent-definitions branch November 19, 2019 06:15
Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Thanks @giginet. I've left a couple of comments. Can you also add a localized intent definition to the TestProject fixture. Would you be able to do a followup PR, sorry for the late review

@@ -11,6 +11,7 @@
- Improved variable expansion runtime [#704](https://github.com/yonaskolb/XcodeGen/pull/704) @rcari
- Fixed missing headers for static framework targets [#705](https://github.com/yonaskolb/XcodeGen/pull/705) @wag-miles
- Using more file types from XcodeProj for PBXFileReferences resulting in less project diffs [#715](https://github.com/yonaskolb/XcodeGen/pull/715) @yonaskolb
- Fixed localized *.intentdefinition have to be added to build source phases [#720](https://github.com/yonaskolb/XcodeGen/pull/720) @giginet
Copy link
Owner

Choose a reason for hiding this comment

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

This * can make anything following render incorrectly in Markdown. Please wrap in `

@@ -81,8 +81,8 @@ class SourceGenerator {
_ = try getSourceFiles(targetType: .none, targetSource: TargetSource(path: path), path: fullPath)
}

func generateSourceFile(targetType: PBXProductType, targetSource: TargetSource, path: Path, buildPhase: TargetSource.BuildPhase? = nil) -> SourceFile {
let fileReference = fileReferencesByPath[path.string.lowercased()]!
func generateSourceFile(targetType: PBXProductType, targetSource: TargetSource, path: Path, buildPhase: TargetSource.BuildPhase? = nil, fileRefenrece: PBXFileElement? = nil) -> SourceFile {
Copy link
Owner

Choose a reason for hiding this comment

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

typo fileRefenrece

@kikettas
Copy link

Nicely done @giginet! Looking forward to seeing this in an upcoming release! 🚀

@yonaskolb
Copy link
Owner

The above comments have been resolved on master 👍

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.

localised .intentdefinition to sources does not work
5 participants