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

Supports specifying multiple package products #1395

Merged
merged 7 commits into from
Sep 11, 2023

Conversation

simonbs
Copy link
Contributor

@simonbs simonbs commented Aug 30, 2023

Adds support for specifying multiple products for when adding a package dependency to a target.

With this change dependencies that were previously written as

dependencies:
- package: FooFeature
  product: FooDomain
- package: FooFeature
  product: FooUI

can now be written as

dependencies:
- package: FooFeature
  products: 
  - FooDomain
  - FooUI

This makes it easier to add multiple products of a single package.

Copy link
Collaborator

@freddi-kit freddi-kit left a comment

Choose a reason for hiding this comment

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

@@ -646,6 +646,7 @@ targets:

**Package dependency**
- [ ] **product**: **String** - The product to use from the package. This defaults to the package name, so is only required if a Package has multiple libraries or a library with a differing name
Copy link
Collaborator

@freddi-kit freddi-kit Aug 30, 2023

Choose a reason for hiding this comment

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

IMO, we can deprecate product and we can set products as standard way.

@yonaskolb what do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

I think we have to keep it around due to it's ability to set different options related to the dependency itself, like you mention below

Comment on lines +675 to +679
dependencies:
- package: FooFeature
products:
- FooDomain
- FooUI
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can we handle if we want to do below case?

  • FooDomain: embed = true
  • FooUI: embed = false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking was that people would use product when needing fine-grained control and products (with an s) when products are to be added the same way.

dependencies:
- package: FooFeature
  product: FooDomain
  embed: true
- package: FooFeature
  product: FooUI
  embed: false

Alternatively, products should be objects as shown below while also supporting products as plain strings. If both aren't supported, the benefit of products become too little.

So both of these would be valid:

dependencies:
- package: FooFeature
  products:
  - name: FooDomain
    embed: true
  - name: FooUI
    embed: false
dependencies:
- package: FooFeature
  products:
  - FooDomain
  - FooUI

Copy link
Owner

Choose a reason for hiding this comment

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

@simonbs the embed property is part of the Dependency type, so the list of objects would break that relationship.
I think having to split out into seperate product is fine. Would like to see that perhaps mentioned in the docs under product like "Useful if you want to define different linking options per product. "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yonaskolb That's a good idea. I've added it in 9785749.

@simonbs
Copy link
Contributor Author

simonbs commented Aug 30, 2023

👍

Please add some test case under Fixtures, like: https://github.com/yonaskolb/XcodeGen/blob/master/Tests/Fixtures/SPM/project.yml

Sure! I'v added that in be92f16.

@yonaskolb
Copy link
Owner

Looks good! Thanks @simonbs!

# Conflicts:
#	Tests/Fixtures/SPM/SPM.xcodeproj/project.pbxproj
#	Tests/Fixtures/SPM/project.yml
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 @simonbs!

@yonaskolb yonaskolb merged commit 486df5d into yonaskolb:master Sep 11, 2023
2 checks passed
@simonbs simonbs deleted the feature/multiple-package-products branch September 11, 2023 08:00
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