Skip to content

NibReusable -> Reusable & NibLoadable#37

Merged
AliSoftware merged 4 commits into
AliSoftware:masterfrom
nekrich:feature/Reusable_NibLoadable
Dec 5, 2016
Merged

NibReusable -> Reusable & NibLoadable#37
AliSoftware merged 4 commits into
AliSoftware:masterfrom
nekrich:feature/Reusable_NibLoadable

Conversation

@nekrich

@nekrich nekrich commented Nov 28, 2016

Copy link
Copy Markdown
Contributor

Hi.

There is no need in strict NibReusable protocol conforming in register functions. You can use protocol composition type.

@AliSoftware

Copy link
Copy Markdown
Owner

Thanks for this PR!

Yup, that convenience NibReusable protocol was added for convenience both for the consumer (easier to make your tableView cells conform to a single protocol) and clarity when reading the source.
But that was before Swift 3, when the syntax was still protocol<Reusable, NibLoadable> which was a bit verbose to use. Now that the syntax has changed in Swift 3 that convenience protocol isn't really needed anymore indeed 😉

@AliSoftware AliSoftware left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you just please:

  • Update the README file accordingly (some documentation references that convenience protocol so if we get rid of it we should update the doc too)
  • Add an entry in the CHANGELOG to mention the change and credit yourself

Thanks!

@nekrich nekrich force-pushed the feature/Reusable_NibLoadable branch from a5fcc90 to f8c262b Compare November 28, 2016 20:45
@nekrich

nekrich commented Nov 28, 2016

Copy link
Copy Markdown
Contributor Author

I think that you should leave NibReusable as syntax sugar.

@AliSoftware

Copy link
Copy Markdown
Owner

I think that you should leave NibReusable as syntax sugar.

🤔 But then what's the point of this PR, if you don't mind me asking?

@nekrich

nekrich commented Nov 29, 2016

Copy link
Copy Markdown
Contributor Author

If you will make a subclass of Reusable cell and make it NibLoadable you will get error on register of subclass. Because it doesn't conforms to NibReusable, but if you try to conform it to NibReusable you will get error that your subclass conforms to Reusable protocol twice.

Or you just make a cell that is Reusable, NibLoadable and you will get an error too.

That is the point of PR.

Lets create cell with theme.

class MyCellWithTheme: UITableViewCell, Reusable {
  var theme: Theme {
    didSet {
      setupTheme()
    }
  }
  func setupTheme() {
     // set bg color, fonts for default labels
  }
}

And this will not work in 3.0.0, but succeeds in my PR.

// Subclass
class MyCustomCellWithTheme: MyCellWithTheme, NibLoadable {
  @IBOutlet private var doStuffbutton: UIButton!
  override func setupTheme() {
    super.setTheme(theme)
     // style for button
  }
}

// Two protocols conforming
class MyAnotherCell: UITableViewCell, Reusable, NibLoadable {
}

And it will not brake existing API, so there is no need to deprecate NibLoadable protocol, and now it can be used as syntax sugar (no strict conforming).

// And this will work too
class MyAnotherCell: UITableViewCell, NibReusable {
}

@AliSoftware

AliSoftware commented Nov 29, 2016

Copy link
Copy Markdown
Owner

Ok, I think I understand, but what surprises me then is that doing this change:

-  final func register<T: UICollectionViewCell>(cellType: T.Type) where T: NibReusable {
+  final func register<T: UICollectionViewCell>(cellType: T.Type) where T: Reusable & NibLoadable {

Would change anything in your scenario… I mean, isn't it the same to conform to NibReusable (which is convenience for Reusable & NibLoadable) or to conform to Reusable & NibLoadable?

Or maybe if it is different — because NibReusable is a separate protocol conforming to both protocols and not strictly a & protocol composition — it would be simpler if that PR just changed protocol NibReusable: Reusable, NibLoadable to typealias NibReusable = Reusable & NibLoadable?

@nekrich

nekrich commented Nov 29, 2016

Copy link
Copy Markdown
Contributor Author

Yep, it is different — because NibReusable is a separate protocol.

😱 typealias for protocol composition it's brilliant idea.

Reusable & NibLoadable equals to NibReusable in both cases
a)typealias NibReusable = Reusable & NibLoadable. Reverse equatable.
b) protocol NibReusable: Reusable, NibLoadable. Reusable, NibLoadable object can't conform to NibReusable.

But I think conformance to 2 protocols Reusable & NibLoadable in func definitions is better than conformance to one typealias NibReusable it clearly shows a list of base protocols to which you should conform.
Cause at first look at this function (as a new programmer who uses this framework at the first time) you will see, that you need to implement 2 protocols in your class.

final func register<T: UICollectionViewCell>(cellType: T.Type) where T: Reusable & NibLoadable

And in the case below you will see one protocol, but in fact there are two of them, so you should check both of them anyway, but it implicit.

final func register<T: UICollectionViewCell>(cellType: T.Type) where T: NibReusable

I'm agree that those protocols conform-and-go, but I prefer to check all frameworks used by me (especially small ones) for what is under the hood.

@AliSoftware

Copy link
Copy Markdown
Owner

Cool, let's go for this then! 👍

  • Using typealias NibReusable instead of protocol NibReusable, but still keeping it (both for retro-compatibility and convenience for users of the lib)
  • Switching to explicit use of Reusable & NibLoadable in the function implementations is ok for me if you think it's more clear

* both `NibLoadable` + `Reusable` protocols.
*/
final class MyXIBIndexSquaceCell: UICollectionViewCell, NibReusable {
final class MyXIBIndexSquaceCell: UICollectionViewCell, Reusable, NibLoadable {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Now that we've decided to keep NibReusable as a typealias, maybe keep the usage of NibReusable in the example project here, especially since it's already explained in the comment that it's in fact just a convenience name combining both.

btw, you might also want to check the doc-comments throughout the code and sample project where we describe NibReusable as "just a convenience protocol that combines both NibLoadable + Reusable protocols" and change it to say it's a "convenience typealias combining NibLoadable & Reusable" instead.

Comment thread CHANGELOG.md Outdated
## UNRELEASED

* Removed strict `NibReusable` protocol conforming in `register` functions.
You can now make `Reusable` cell, and `NibLoadable` subclass.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please add 2 spaces after your full-stop here (see other entries for inspiration).
Ending a line with two spaces in Markdown allows to go to the next line in the same paragraph (like a <br>) in the rendered document.


The typical formatting I use in the CHANGELOG is this, where represent a space:

* Change description.••
  [@your_github_handle](link_to_your_gh_profile)
  [#PR_Num](link_to_this_pr)

Comment thread README.md Outdated

* Use the `Reusable` protocol if they don't depend on a NIB (this will use `registerClass(…)` to register the cell)
* Use the `NibReusable` protocol if they use a `XIB` file for their content (this will use `registerNib(…)` to register the cell)
* Use the `NibReusable` protocol (aka combination of `Reusable` & `NibLoadable` protocols) if they use a `XIB` file for their content (this will use `registerNib(…)` to register the cell)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Use the NibReusable typealias

Updated README.md & CHANGELOG.md.
@nekrich nekrich force-pushed the feature/Reusable_NibLoadable branch from 2a22c19 to 416d505 Compare November 29, 2016 19:56
Comment thread README.md

* Use the `Reusable` protocol if they don't depend on a NIB (this will use `registerClass(…)` to register the cell)
* Use the `NibReusable` protocol (aka combination of `Reusable` & `NibLoadable` protocols) if they use a `XIB` file for their content (this will use `registerNib(…)` to register the cell)
* Use the `NibReusable` typealias if they use a `XIB` file for their content (this will use `registerNib(…)` to register the cell)

@AliSoftware AliSoftware Nov 29, 2016

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I found it kinda nice to add the information/aka you added before, like Use the NibReusable typealias (equivalent to Reusable & NibLoadable) if …

@AliSoftware

AliSoftware commented Dec 2, 2016

Copy link
Copy Markdown
Owner

🤔 That's the 4th time I relaunch the Travis-CI build because each time it errors (it doesn't launch the job and fail, no, it just errors without even launching the build).

I have no idea if that's linked to the shortage that Travis-CI is currently experiencing with OSX jobs or not, so if we should change something to make it build again or not, I'm a bit confused…

(same problem with #38)

@AliSoftware

Copy link
Copy Markdown
Owner

Ok, still don't know why the Travis job keeps erroring, and I have no idea how to investigate further… so merging anyway after testing it locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants