Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Add explicit bundle parameter to support frameworks #17

Merged
merged 10 commits into from
Feb 15, 2017

Conversation

djbe
Copy link
Member

@djbe djbe commented Feb 5, 2017

Carry over changes from SwiftGen/SwiftGen#255 with some added fixes for Swift 2.3. I've also implemented the same changes for storyboards.

This still needs some work to add this to the other commands, for example:

  • images: the way to load assets from catalogs in bundles is different between iOS (UIImage(named:in:compatibleWith:)) and macOS (bundle.image(forResource:))
  • fonts: the fonts need to be registered using CTFontManagerRegisterFontsForURL before use. I've done this using an override of initialize() in a Font extension, but we could also just check in our init if it's already loaded. Would love some input on the preferred method for this.

@djbe djbe changed the title Add explicit bundle parameter to support framewors Add explicit bundle parameter to support frameworks Feb 5, 2017
@djbe
Copy link
Member Author

djbe commented Feb 5, 2017

Do note I first want to merge the other PRs related to linting and compilation, to avoid further issues with the output code.

@AliSoftware
Copy link
Collaborator

I like that change and approve this PR, but wouldn't it a breaking change, because Image now doesn't have an init(name: Asset) anymore with that PR, so people relying on that wouldn't be able to use it and their code will break?

  • So either we mark that PR as breaking and only merge it as part of 5.0 as the default image template
  • Or we merge it but make the images template a separate template different from the default, but I think it should then be the default in 5.0, so not sure it's worth this intermediate step
  • Or we restore the init again… at least for iOS (which should be possible because it still invokes an init, while for OSX I'm guessing the problem was that to build that NSImage from a bundle you had to use a method on NSBundle and not an init and Swift wasn't happy with a convenience init not calling another init?)

@NachoSoto
Copy link

Oh yeah I didn't notice that. I'd vote for keeping the initializer.

@djbe djbe force-pushed the feature/bundle-token branch from 9de94a6 to 51bfc6b Compare February 13, 2017 12:23
@djbe
Copy link
Member Author

djbe commented Feb 13, 2017

Yeah, in Swift you can't return a different object in the init like in Objective-C 😞

I've re-added the init for both platforms. On macOS it falls back to the old behaviour.

@djbe djbe force-pushed the feature/bundle-token branch from 51bfc6b to 4c4a611 Compare February 13, 2017 12:35
@AliSoftware
Copy link
Collaborator

Cool!

One last question, I'm wondering if the ImageConvertible protocol is worth it? I mean, don't get me wrong, I love Mixins obviously. But given that it means declaring a protocol so adding ImageConvertible in the global namespace, that means a new type and name that people won't be able to use in their own code for other stuff (e.g. if they already had a protocol named ImageConvertible in their own codebase, it would conflict)

It's not much, but given that this Mixin isn't necessary per se and generating the init directly doesn't change the amount of code or the complexity, maybe it's not worth it risking a name conflict?

@djbe djbe force-pushed the feature/bundle-token branch from 38ce5ed to cf66111 Compare February 13, 2017 17:06
@AliSoftware
Copy link
Collaborator

@djbe now that inits are back, this should be mergeable right now, even in upcoming 4.2, as it isn't breaking anymore, right?

@djbe
Copy link
Member Author

djbe commented Feb 15, 2017

Jup, should be 👌

@AliSoftware AliSoftware merged commit 23cc918 into master Feb 15, 2017
@AliSoftware AliSoftware deleted the feature/bundle-token branch February 15, 2017 23:29
@djbe djbe modified the milestone: 1.0.0 Feb 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants