Renamed storyboard property to unified sceneStoryboard

[ghost]

Current implementation for instantiating VC from storyboard is not working correctly.
Self().storyboard from StoryboardSceneBased.swift is creating new instance of VC and then reading "storyboard" property. However value of read property is not from StoryboardBased.storyboard (which is static) but from UIViewController.storyboard (which is optional).
I'm proposing small change (but it's breaking API change) by renaming property from 'storyboard' to 'sceneStoryboard' for avoid conflicts and make code working correctly.

[AliSoftware]

Hey, thanks for the PR!

I don't understand though why you would do Self(). storyboard (which would indeed invoke the Self.init() then call .storyboard on the instanciated VC) if your wish is to know in which storyboard a VC class is designed, and thus want to invoke the static method?
If your wish is to invoke the static method you need to call that method on the type, not on a newly created instance, right? So just Self.storyboard (with no parentheses)? Or am I misunderstanding something?

[AliSoftware]

So for me the only change to do is on line 45 where indeed there's this extra () that shouldn't be there (nice catch!) But I'm not sure it's worth changing the name too (at this will need a major version bump instead of just a bugfix version)

[AliSoftware]

(To me those two lines after the only one needing to be changed, to fix the bug you mention)

[ghost]

Exactly. Removing Extra () solving problem. However compiler is raising an issue when you trying to call self.storyboard from instantiate() function (again it's pointing to UIViewController.storyboard instead to StoryboardBased.storyboard).
Its possible to add default implementation for StoryboardSceneBased like this:

public extension StoryboardSceneBased {
  static var sceneIdentifier: String {
    return String(describing: self)
  }
  static var storyboard: UIStoryboard {
    return UIStoryboard(name: "foo", bundle: nil)
  }
}

to switch Swift compiler to use dedicated static method, but it's dumb workaround.

After fail of all my tries with 'self' combinations to pass compilation I proposed to rename property name.

This issue also affects example attached to library (instantiate Details View Controller after tapping Details button in InfoCell 0 or 1)

[AliSoftware]

Ah, right. Then shouldn't we use Self.storyboard (And not self.storyboard) in instantiate() then?

I agree that renaming the property to avoid confusion would be a good idea but as it's also a breaking change (and thus requires to bump the major version) I'm trying to avoid it just to solve a bug 😉

[AliSoftware]

What do you say we split this PR in two?

• one PR to just Fix the bugs (remove () in call to Self.storyboard, use Self.storyboard in instantiate, …) so we can just do a 3.0.1 bug fix release
• another PR after that to rename the property to sceneStoryboard in a separate release that will bump the version to 4.0.0

We could also keep the stotyboard property in 3.0.1 but mark it with the deprecated attribute to point to the new one, and remove it altogether in 4.0.0

[Grubas7]

@jakubgert Nice catch! But we could avoid such problem if we would have unit tests. Could you add them for your code?

[ghost]

@AliSoftware as I mentioned before,

one PR to just Fix the bugs (remove () in call to Self.storyboard

This will not work because of Swift will not allow to to this without default protocol implementation.

another PR after that to rename the property to sceneStoryboard in a separate release that will bump the version to 4.0.0

That sounds good. But with UT @Grubas7

[AliSoftware]

@jakubgert I'm not sure I understand why a PR just to fix the bug wouldn't work?
A StoryboardSceneBased is always guaranteed to have a static let storyboard type property no matter what. No need for a default implementation of it, since every type conforming to it will have to provide an implementation anyway. The fact that the StoryboardSceneBased protocol declares the static var storyboard: UIStoryboard { get } property to be a requirement for all conforming types should enough, even without a default implementation. That's how protocols work, right?

[ghost]

@AliSoftware

That's how protocols work, right?

Exactly, that protocols works, but in this particular case after change from: Self().storyboard (which was wrong) to Self.storyboard (where expected is to call static var storyboard: UIStoryboard getter) I have Swift Compiler error:
Instance member 'storyboard' cannot be used on type 'Self'
Swift compiler issue on two same variable names, but different types (static StoryboardSceneBased.storyboard and optional UIViewController.storyboard)???

After rename static var storyboard: UIStoryboard to any other everything works fine.

I hope this is more clear. I should mention about it at the beginning.

[AliSoftware]

Ah, I see it now, thx for the detailed explanation.

We should file a bug report at bugs.swift.org about that btw (the fact that it only see the storyboard instance property on UIViewController but not the class variable on StoryboardSceneBased.

In the meantime, I managed to make it compile by adding some hints to the compiler, using this:
let storyboard = (Self.self as StoryboardSceneBased.Type).storyboard (and remove the ? after storyboard in the next line)

I'm sorry to insist on doing it in two parts, but semantic versioning and API change means that a lot of people won't probably upgrade to the future Reusable 4.0.0 just yet and won't have the bug fix very soon if we only fix it in 4.0.0, so… 😉

[ghost]

This is working:
let storyboard = (Self.self as StoryboardSceneBased.Type).storyboard
I can fix PR and submit it again if you want.

Regarding bug on Swift, I'm working on small example which I want to attach with description and submit. It will be ready soon.

[AliSoftware]

Would love to!

Thanks again 👍

[AliSoftware]

@jakubgert Any news on splitting this PR so that we can at least merge the bug-fix as a minor version bump?

[ghost]

Apologies for delay, I was really buy last week.
I submitted new PR with fix which is 100% compatible with current existing API (so no breaking changes here: #38).
This PR we can close.

[AliSoftware]

Cool. Waiting for you to add a CHANGELOG entry in #38 so I can merge it quickly thn (even if Travis is having issues recently and the PR validation by Travis might take some time).

I'll keep this #33 open for now, as once #38 is merged we could just rebase this PR #33 on top of it to keep the renaming (but only merge it as a new, breaking major version after the release of the minor version including #38)

[AliSoftware]

@jakubgert Any chance you could rebase this PR on top of master so we can have this renaming done and publish a new major release?

Thx!

[AliSoftware]

Merged via 7d5216d

[AliSoftware]

For reference: https://bugs.swift.org/browse/SR-4615