-
Notifications
You must be signed in to change notification settings - Fork 39
feat: supportsResources returns true if product is staticFramework #84
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
Conversation
fortmarek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Let's wait with the merge until the accompanying Tuist PR is merged: tuist/tuist#7006
| case .staticFramework: | ||
| return containsResources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we just return true? supportsResources and containsResources are two different things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've suggested on tuist/tuist#7006 to make .staticFramework embeddable only when it contains resources.
The easiest way to do so in the tuist's codebase is to consider .staticFramework as a framework that supports resources only when it really contains it.
Do you have doubts about this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should still embed .staticFramework only if it contains resources. But this variable should always return true for .staticFramework, otherwise, we're changing the meaning of this variable for saving ourselves a couple of lines.
I'd check for containsResources in the place where we make the decision whether .staticFramework should be embedded or not.
| /// Returns true if the target contains resources or CoreData models | ||
| public var containsResources: Bool { | ||
| !resources.resources.isEmpty || !coreDataModels.isEmpty | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep this inside TuistCore as an extension. We're trying to keep the API surface of XcodeGraph smaller to make it easier not to make breaking changes here and to limit the need for cross-repo PRs.
555b8af to
ba69c2d
Compare
ba69c2d to
7ae5590
Compare
Needed for tuist/tuist#7006
Target.supportsResourcesreturnstrueifTarget.product == .staticFramework.