Skip to content

Add "import" option, which can be used to add (multiple) import statements#175

Merged
dostrander merged 15 commits intoSwiftGen:masterfrom
djbe:feature/storyboards-import
Nov 9, 2016
Merged

Add "import" option, which can be used to add (multiple) import statements#175
dostrander merged 15 commits intoSwiftGen:masterfrom
djbe:feature/storyboards-import

Conversation

@djbe
Copy link
Copy Markdown
Member

@djbe djbe commented Sep 22, 2016

Useful if you have view controllers which are classes from libraries.

For example, if you have a library A that provides a view controller class B, and instantiate that class in your storyboard, then the swiftgen generated file will reference class B. Problem is that swift won't be able to find B, unless you add import A at the top of the file.


The provided solution should allow adding multiple import statements, but I honestly haven't been able to test it because:

  • I'm using Xcode 8
  • The swift 2.3 branch doesn't work for me. It compiles, but I get an error force unwrapping a nil optional in CommandRunner.swift (let executableName = parser.shift()!)

@AliSoftware
Copy link
Copy Markdown
Collaborator

This looks good!

Now that #179 has been merged and that SwiftGen's code is now using Xcode 8, this should now be testable at last!

Copy link
Copy Markdown
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Ok could you please:

  • Add at least one Unit Tests for each command (you can duplicate an existing Unit Test, change the call to GenumKit so it pass some values in extraImports, and add fixtures containing those extra import statements)
  • add an entry in the CHANGELOG to mention this new feature and credit yourself

Otherwise seems 👌 ! Thx 😉

@djbe
Copy link
Copy Markdown
Member Author

djbe commented Nov 6, 2016

Allright, I've added one test (on iOS) + modified the "all" tests as there's a new storyboard.

Let me know if the test I've made is correct/how you wanted it, and I'll do the same change(s) for the macOS tests.

CHANGELOG.md Outdated

* Add option to add import statements at the top of the generated swift file (for
storyboards) using the `import` flag. Multiple imports can be added by repeating
this flag.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you add two spaces after the full-stop at the end of this line, like in other entries?

(This allows the rendered markdown to go on the next line in the same paragraph)

@AliSoftware
Copy link
Copy Markdown
Collaborator

AliSoftware commented Nov 6, 2016

This first test looks good to me 👌 maybe except that it would be nice to be consistent in the imports vs the VC used? Like use the example of an extraImport SlackTextViewController and an actual SlackTextViewController in the storyboard file, for consistency's sake — even if the test fixtures just compare generated texts without trying to compile them, it would be more logical and comprehensive for someone reading the test fixtures)

Other than that seems ok so you can continue add the other tests for the other templates and we'd be able to merge this 👍

@djbe
Copy link
Copy Markdown
Member Author

djbe commented Nov 6, 2016

Oh, that explains why they were appearing on 1 line, I had no idea.

I'll modify the test and add the macOS ones.

Sent from my iPhone

On 6 Nov 2016, at 22:57, AliSoftware notifications@github.com wrote:

This first test looks good to me 👌 maybe except that it would be nice to be consistent in the imports vs the VC used? Like use the example of an extraImport SlackTextViewController and an actual SlackTextViewController in the storyboard file, for consistency's sake even if the test fixtures just compare generated tests that seems more logical to read)

Other than that seems ok so you can continue add the other tests for the other templates and we'd be able to merge this 👍


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@djbe
Copy link
Copy Markdown
Member Author

djbe commented Nov 7, 2016

Bump (I have no idea if you get notified when I push changes)

@AliSoftware
Copy link
Copy Markdown
Collaborator

I do ^^

Will take a look later when I'm at my computer

extension UIViewController {
func performSegue<S: StoryboardSegueType>(segue: S, sender: AnyObject? = nil) where S.RawValue == String {
performSegue(withIdentifier: segue.rawValue, sender: sender)
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

#194 is probably going to land soon. You may want to reflect what is in there as well and merge as it comes to the performSegue:

https://github.com/AliSoftware/SwiftGen/pull/194/files#diff-5eb3bf790669fb7ada0dfec0ac55ca5cR39

@AliSoftware
Copy link
Copy Markdown
Collaborator

Looks good.

Bad news, now that the changes in #194 has correctly been merged in here, that's #197 (AnyObject -> Any in performSegue) — which is also close to merging too — that will enter in conflict now 😆

@dostrander I'm personally OK to merge both #197 and this #175, can I let you merge both in the order you prefer and let you fix the likely conflicts between the two on that performSegue during the dual-merge? (Even if I think that if you merge #175 first and #197 next in that order, this might go well all by itself?)

@dostrander
Copy link
Copy Markdown
Collaborator

@AliSoftware Yeah sounds good i'll take care of it :)

@AliSoftware
Copy link
Copy Markdown
Collaborator

Thx 👌

@dostrander dostrander merged commit c715d37 into SwiftGen:master Nov 9, 2016
@AliSoftware
Copy link
Copy Markdown
Collaborator

👍

@djbe djbe deleted the feature/storyboards-import branch November 12, 2016 14:47
@djbe djbe added this to the 3.0.2 (or 3.1.0) milestone Nov 15, 2016
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