Skip to content

Added support for dependency destination specification. (Resolves #1038)#1039

Merged
yonaskolb merged 7 commits intoyonaskolb:masterfrom
JakubBednar:master
Jul 15, 2021
Merged

Added support for dependency destination specification. (Resolves #1038)#1039
yonaskolb merged 7 commits intoyonaskolb:masterfrom
JakubBednar:master

Conversation

@JakubBednar
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
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.

Hello, @JakubBednar. Before submitting a pull request, please add a test to it. Please refer to the test topic in CONTRIBUTING.md https://github.com/yonaskolb/XcodeGen/blob/master/CONTRIBUTING.md#tests

@JakubBednar
Copy link
Copy Markdown
Contributor Author

JakubBednar commented Mar 24, 2021

Hello, frankly it is harder for me to navigate your unit-tests than to update the actual code and add the feature. :) So I was hopping to get a general feedback on the proposed feature and solution first. If you'd agree that this is something that makes sense to you and the proposed way of implementation is correct as well I will add the tests. But if you decide that you do not want this feature or you want it implemented differently than I would be just wasting my time on the tests. (I did test it on my use-case and generated my project with this. Embedding command-line-tools and frameworks to custom locations)

With that said, I suppose I should extend both ProjectSpecTests and PBXProjGeneratorTests. Or is there anything else?

@JakubBednar JakubBednar force-pushed the master branch 3 times, most recently from d4a8bcb to 719256f Compare March 27, 2021 09:04
…current embeding then the new one with custom copy spec. (yonaskolb#1038)
@freddi-kit
Copy link
Copy Markdown
Collaborator

Thank you for your update and I'm sorry for my late reply. Now I became Collaborator for this repo so please give a time to learn and I'll review it soon!

@JakubBednar JakubBednar requested a review from freddi-kit April 10, 2021 10:53
@JakubBednar
Copy link
Copy Markdown
Contributor Author

Hello, @freddi-kit. Anything I can do to make this get merged?

@jakubkiermasz-zd
Copy link
Copy Markdown

Hello!
Seems like this resolves #1102, doesn't it :)?

@JakubBednar, @freddi-kit any updates?

@jakubkiermasz-zd
Copy link
Copy Markdown

If you are interested, https://github.com/zendesk/XcodeGen is the fork aligned with the develop that we will use till it's merged.

@yonaskolb
Copy link
Copy Markdown
Owner

Thanks for the great work here @JakubBednar. It all seems good. Only thing is I'm a little hesitant to merge 1700 lines of testing code that will have to be maintained. If you can reduce this a bit with the use of some helper functions or some setup objects that would be great. Once done and the conflicts are resolved, we can get this merged 👍

@jakubkiermasz-zd are you confirming this solves your particular use case?

@freddi-kit
Copy link
Copy Markdown
Collaborator

freddi-kit commented Jun 30, 2021

Ah sorry for not adding comments for a long time, I thought I asked others to review but it looks ask review button in github not worked when I tapping, sorry. 🙇

@jakubkiermasz-zd
Copy link
Copy Markdown

jakubkiermasz-zd commented Jun 30, 2021

@yonaskolb I am. It works like a charm ^^

Regarding the conflicts: you can use the fork I've posted above ^^

@JakubBednar
Copy link
Copy Markdown
Contributor Author

JakubBednar commented Jul 13, 2021

I have resolved the conflicts and removed IMO the only boiler-plate code in the unit-tests. @yonaskolb , please note the following about the unit-tests:

  1. I did cover all available dependency types and there is a lot of them. Therefore a lot of tests.
  2. For each dependency type, the first test actually fixes how xcodegen worked before changes. For some dependency types it ignored embed: true for some it ignored embed: false. I did not change the default behaviour without specifying the custom copy phase. The only target type that still ignores custom copy phase is the Bundle dependency type. See the last two unit-tests.

CHANGELOG.md Outdated
## 2.20.0

#### Added
- Allow specifying a `copy` setting for each dependency. [#1038](https://github.com/yonaskolb/XcodeGen/pull/1039) @JakubBednar
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can we move this up into Next Version

Copy link
Copy Markdown
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.

Great, thank you @JakubBednar

@yonaskolb yonaskolb merged commit d35d22f into yonaskolb:master Jul 15, 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.

4 participants