Assigning rows to RadioSection & Row protocol equatibility #14
Conversation
…es equatable compliance of Row protocol
Source/Protocol/Row.swift
Outdated
| } | ||
|
|
||
| /// Returns true iff `lhs` and `rhs` have equal titles and subtitles. | ||
| func ==(lhs: Row, rhs: Row) -> Bool { |
There was a problem hiding this comment.
Explicit Top Level ACL Violation: Top-level declarations should specify Access Control Level keywords explicitly. (explicit_top_level_acl)
Operator Function Whitespace Violation: Operators should be surrounded by a single whitespace when defining them. (operator_whitespace)
There was a problem hiding this comment.
This causes a build error. Can you explain a little bit more about why this change is needed?
There was a problem hiding this comment.
You need a to declare the == function as top level in order to compare two different Row items. It looks like to resolve the build error we must also have the non-top level declaration.
| @@ -49,7 +49,7 @@ open class RadioSection: Section { | |||
| return options | |||
| } | |||
| set { | |||
| options = rows as? [OptionRowCompatible] ?? options | |||
| options = newValue as? [OptionRowCompatible] ?? options | |||
Source/Protocol/Row.swift
Outdated
| } | ||
|
|
||
| /// Returns true iff `lhs` and `rhs` have equal titles and subtitles. | ||
| func ==(lhs: Row, rhs: Row) -> Bool { |
There was a problem hiding this comment.
This causes a build error. Can you explain a little bit more about why this change is needed?
Codecov Report
@@ Coverage Diff @@
## develop #14 +/- ##
========================================
Coverage 89.32% 89.32%
========================================
Files 14 14
Lines 309 309
========================================
Hits 276 276
Misses 33 33
Continue to review full report at Codecov.
|
Source/Protocol/Row.swift
Outdated
|
|
||
| /// Returns true iff `lhs` and `rhs` have equal titles and subtitles. | ||
| public func == (lhs: Row, rhs: Row) -> Bool { | ||
| return lhs.title == rhs.title && lhs.subtitle == rhs.subtitle |
There was a problem hiding this comment.
I just realised why you'd like to add this. Perhaps it's because the Row protocol doesn't conform to Equatable. By adding this global function, there'd be some cases like the following:
let a: Row = TapActionRow(title: "same title", action: nil)
let b: Row = SwitchRow(title: "same title", switchValue: false, action: nil)
print(a == b) // true, but they shouldn't be equal.This makes me wonder if the Row extension providing a default == implementation was misleading in the first place.
Since all row classes (except TapActionRow) implement their own == implementation, this Row extension can probably be removed. Therefore TapActionRow should implement its own Equatable conformance.
What do you think?
There was a problem hiding this comment.
Well my reason for doing this was to add a slight bit more dynamism to the library, I used it to traverse the tableContents matching a Row item to get the indexPath, use that to get the cell, and then call customize() from the row item to that cell.
What might make it even better would be to simply declare the Row protocol as a class protocol. That would allow for identity checks which would be even better.
There was a problem hiding this comment.
I think this global == function would be better declared in your project's scope where you need it. Having the Row as a class protocol sounds like a good solution to me. 👍
Source/Protocol/Row.swift
Outdated
| /// Returns true iff `lhs` and `rhs` have equal titles and subtitles. | ||
| public static func == (lhs: Self, rhs: Self) -> Bool { | ||
| return lhs.title == rhs.title && lhs.subtitle == rhs.subtitle | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
Source/Protocol/Row.swift
Outdated
| } | ||
|
|
||
|
|
||
| extension Row { | ||
|
|
||
There was a problem hiding this comment.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
Source/Protocol/Row.swift
Outdated
| /// A closure related to the action of the row. | ||
| var action: ((Row) -> Void)? { get } | ||
|
|
||
There was a problem hiding this comment.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
Source/Protocol/Row.swift
Outdated
| /// The subtitle text of the row. | ||
| var subtitle: Subtitle? { get } | ||
|
|
||
There was a problem hiding this comment.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
Source/Protocol/Row.swift
Outdated
| /// The title text of the row. | ||
| var title: String { get } | ||
|
|
||
There was a problem hiding this comment.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
Source/Protocol/Row.swift
Outdated
| public protocol Row { | ||
|
|
||
| public protocol Row: class { | ||
There was a problem hiding this comment.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
|
Thanks for your contribution to the project! |
fixes issue when dynamically assigning rows in RadioSection, also fixes equatable compliance of Row protocol