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 ability to define templateAttributes within a target #533

Merged
merged 1 commit into from Mar 17, 2019

Conversation

tomquist
Copy link
Contributor

@tomquist tomquist commented Mar 13, 2019

This allows parameterizing templates. One specific problem I wanted to solve with this issue is that I have a multi-framework project where framework-targets are specified by a target template. Each framework consists of the framework itself and a corresponding test target. The framework template is specified like this:

targetTemplates:
  Module:
    platform: iOS
    type: framework
    sources: [modules/$target_name/Sources]

Now I'd like to create a template for the test target where the test-files are placed in modules/<ModuleName>/Tests. I have no way of defining a common template for this.

When this PR is merged, I'm able to define the following template:

targetTemplates:
  ModuleTests:
    platform: iOS
    type: bundle.unit-test
    sources: [modules/$targetUnderTest/Tests]
    dependencies:
      target: $targetUnderTest

and use it like this:

targets:
  MyFramework:
    templates: [Module]
  MyFrameworkTests:
    templates: [ModuleTests]
    templateAttributes:
      targetUnderTest: MyFramework

@AlexisQapa
Copy link

AlexisQapa commented Mar 13, 2019

What about going even further and define associated templates for tests into the tested target template ? This would give us the ability to define only targets of interest.

targetTemplates:
Module:
platform: iOS
type: app
sources: [modules/sources]
tests:
template: ModuleTest

use
targets:
MyFramework:
templates: [Module]

I don't know if people use more than a unit (and for app a ui) test targets

@tomquist
Copy link
Contributor Author

I think my solution solves the issue in a more general way and would also be useful for many other use cases. I agree that it would also be a good idea to have some way to automatically generate an associated test target for a framework target but this can still be done separately.

@AlexisQapa
Copy link

That's what I thoughts I'll give this a try when I can and give you feedbacks

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.

Awesome, thank you @tomquist, this is fantastic addition!

I wonder if now that the replacement strings can be anything if the syntax should be a bit more verbose so as not to cause any clashes. For example ${framework_name} instead of $framework_name.
Before we could get away with just the $ as the names of the replacements were known and deemed "safe", now I'm not so sure. If we did use a newer syntax, it might be worth updating to ${target_name} and ${platform} as well, while keeping backwards compatibility. What do you think?

Docs/ProjectSpec.md Outdated Show resolved Hide resolved
@tomquist
Copy link
Contributor Author

Regarding your suggestion to use ${attributeName} instead of $attributeName: I totally agree but didn't want to make the change without discussing first. I'll try to implement that. Ideally xcodegen would show a warning when $target_name or $platform is used to migrate to ${target_name} and ${platform}
Is there anything I can reuse to generate such warnings?

@yonaskolb
Copy link
Owner

yonaskolb commented Mar 14, 2019

That would be great. Warnings would come from a linter which is not yet implemented and tracked here #214

@tomquist
Copy link
Contributor Author

I did the necessary change and also added warnings for old usage of $target_name and $platform. @yonaskolb Could you please have a look at this? I hope my solution is fine for now until it is replaced by the more sophisticated linter solution.

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.

Looks good! It does the job for now. Left a couple of small comments

Docs/ProjectSpec.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Sources/ProjectSpec/SpecValidation.swift Outdated Show resolved Hide resolved
@tomquist tomquist force-pushed the support-template-attributes branch from 029a744 to 3bb7ebd Compare March 17, 2019 10:28
Docs/ProjectSpec.md Outdated Show resolved Hide resolved
Docs/ProjectSpec.md Outdated Show resolved Hide resolved
@tomquist tomquist force-pushed the support-template-attributes branch from 7fb05f0 to 3021da2 Compare March 17, 2019 13:20
@tomquist
Copy link
Contributor Author

I'll rebase the branch on master and add additional tests that combine nested templates with targetAttributes.

This allows parameterizing templates. Also change
placeholder syntax to `${placeholderName}` also for
existing placeholders `$target_name`and `$platform`
and generate warnings when using the old placeholder
syntax.
@tomquist tomquist force-pushed the support-template-attributes branch from 3021da2 to aaae772 Compare March 17, 2019 13:55
@tomquist
Copy link
Contributor Author

I adjusted the parses complex nested target templates spec to also include templateAttributes.

@yonaskolb
Copy link
Owner

Great work!

@yonaskolb yonaskolb merged commit 6620187 into yonaskolb:master Mar 17, 2019
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.

None yet

3 participants