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 Settings.bundle containing acknowledgements if CocoaPods is enabled #191

Closed
wants to merge 1 commit into from
Closed

Add Settings.bundle containing acknowledgements if CocoaPods is enabled #191

wants to merge 1 commit into from

Conversation

bitcrumb
Copy link
Contributor

Add Settings.bundle which contains CocoaPods 'Acknowledgements' section (updated on every pod install) as well as an entry containing the version & build number (updated on each build).

@bitcrumb bitcrumb changed the title Add Settings.bundle which contains CocoaPods 'Acknowledgements' section by default Add Settings.bundle containing acknowledgements & version info Nov 15, 2014
@bitcrumb
Copy link
Contributor Author

Afterthought: I should still add an option to the cli to allow anyone to enable/disable this feature.

@bitcrumb
Copy link
Contributor Author

Did quite a refactor to be able to put everything behind the enable_settings option. Downside is that you lose the flexibility of specifying where the Settings.bundle should go in your project structure (but that's not that big of a tradeoff I guess, except that it assumes a 'Resources' group).

Also note that the CFBundleVersionand the CFBundleShortVersionStringare updated in the Info.plist of the generated build & not in the Info.plist of the project itself, since that would require the ability to insert a shell script build phase before the 'Copy Bundle Resources'. Something which isn't possible at the moment out-of-the-box (so I didn't bother at the moment).

Any feedback & assistance is appreciated @gfontenot, since I am pretty new to Ruby & this project. Keep up the good work, this is an awesome tool and I'd love to contribute even more.

@gfontenot
Copy link
Member

Slick. Thanks for this. I'm wondering if we shouldn't make the acknowledgement bit conditional to CocoaPods being enabled. That would give users a bit more control over the setup of their project. They could (for example) keep the settings bundle enabled, but disable CocoaPods and end up with an empty settings bundle, or they could disable the settings bundle and keep CocoaPods enabled and end up with no attribution.

@bitcrumb
Copy link
Contributor Author

Good point, you're correct that users will now end up with an empty Acknowledgements sections if they decide to disable CocoaPods support. I'll have to look into how I can dynamically include/exclude this from the Root.plist. Or maybe I could supply two different Setting.bundle files in the templates folder (one with and one without the Acknowledgments section) & move one or the other depending on what the user chose. I'll have a look at this later today. Note that the Settings.bundle will never be empty, there's always going to be a Version entry. Should this be optional as well?

@gfontenot
Copy link
Member

You can use ERB for this fairly easy. A conditional block in ERB would look like this:

<% if use_cocoapods %>
# stuff
<% end %>

Note the lack of = in the ERB tag. This would make ERB execute the code, but not print the result.

Regarding the version entry: I feel like I'd prefer to not include that by default. It's something that can easily be overridden in a custom plist locally if needed.

@bitcrumb
Copy link
Contributor Author

Thanks, I'll update this pull request with your feedback as soon as I find the time.

@bitcrumb bitcrumb changed the title Add Settings.bundle containing acknowledgements & version info Add Settings.bundle containing acknowledgements if CocoaPods enable Nov 18, 2014
@bitcrumb
Copy link
Contributor Author

I removed the automatic addition of version info & updated the file manager so that files inside directories get parsed as well (needed to use ERB inside Root.plist of the Settings.bundle).

@@ -88,7 +88,7 @@ def linkable_file?(name)
end

def resource_file?(name)
name.end_with?('xcassets')
name.end_with?('xcassets', '.bundle')
Copy link
Member

Choose a reason for hiding this comment

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

Don't need the . here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

@bitcrumb
Copy link
Contributor Author

Code changes after your feedback have been pushed. Only thing remaining is the code duplication remark. Any suggestions?

@gfontenot
Copy link
Member

None that would be within the scope of this PR. I think a larger refactor of the xcodeproj_helper and project classes is probably needed.

If you can rebase this down into a single commit, I'll get it into master.

@bitcrumb
Copy link
Contributor Author

Done.

@bitcrumb bitcrumb changed the title Add Settings.bundle containing acknowledgements if CocoaPods enable Add Settings.bundle containing acknowledgements if CocoaPods is enabled Nov 19, 2014
@gfontenot
Copy link
Member

Awesome, thanks. Tweaked the commit message a little and merged in a2d9d90

@gfontenot gfontenot closed this Nov 19, 2014
@bitcrumb bitcrumb deleted the feature/settings branch November 24, 2014 09:44
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.

2 participants