-
Notifications
You must be signed in to change notification settings - Fork 818
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
Added support for dependency destination specification. (Resolves #1038) #1039
Conversation
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.
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
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? |
d4a8bcb
to
719256f
Compare
…current embeding then the new one with custom copy spec. (yonaskolb#1038)
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! |
Hello, @freddi-kit. Anything I can do to make this get merged? |
Hello! @JakubBednar, @freddi-kit any updates? |
If you are interested, https://github.com/zendesk/XcodeGen is the fork aligned with the develop that we will use till it's merged. |
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? |
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. 🙇 |
@yonaskolb I am. It works like a charm ^^ Regarding the conflicts: you can use the fork I've posted above ^^ |
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:
|
CHANGELOG.md
Outdated
@@ -66,6 +66,7 @@ | |||
## 2.20.0 | |||
|
|||
#### Added | |||
- Allow specifying a `copy` setting for each dependency. [#1038](https://github.com/yonaskolb/XcodeGen/pull/1039) @JakubBednar |
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.
Can we move this up into Next Version
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.
Great, thank you @JakubBednar
No description provided.