Chain Concatenation with BONTextables #161
Conversation
- Implemented appendLink with BONTextable - Updated example code accordingly
| } | ||
|
|
||
There was a problem hiding this comment.
#req whitespace. Do you have clang-format installed as per the readme? It should catch this.
There was a problem hiding this comment.
Huh, I thought I did. I'll see what's wrong.
|
|
||
| @end | ||
|
|
||
| @interface BONChain (Deprecated) | ||
|
|
||
| @property (copy, nonatomic, readonly) BONChainColor textColor __attribute__((deprecated("use -color instead"))); | ||
|
|
||
| - (void)appendLink:(id<BONTextable>)link separator:(BONNullable NSString *)separator __attribute__((deprecated("use -appendLink:link separatorTextable:separator instead"))); |
There was a problem hiding this comment.
#req the way to spell the name of the new method is -appendLink:separatorTextable:.
|
#nth this all came about because the documentation and the code didn't agree with each other. Was this code tested before you changed it? If so, please add new tests for the new behavior. If not, some tests would be nice to confirm that the new behavior works consistently. |
- Added some test classes that did not have target membership in BonMotTests - Updated existing tests that used deprecated version of appendLink - testAppendingWithDifferentAttributes: in BONTextableTestCase tests the behavior described in the readme
|
|
||
| @end | ||
|
|
||
| @interface BONChain (Deprecated) | ||
|
|
||
| @property (copy, nonatomic, readonly) BONChainColor textColor __attribute__((deprecated("use -color instead"))); | ||
|
|
||
| - (void)appendLink:(id<BONTextable>)link separator:(BONNullable NSString *)separator __attribute__((deprecated("use -appendLink: separatorTextable: instead"))); |
There was a problem hiding this comment.
#req no space: -appendLink:separatorTextable:. That wasn't a typo - it's just a quirk of how Obj-C selector names are spelled.
| @@ -441,4 +441,14 @@ - (BONChainColor)textColor | |||
| return self.color; | |||
| } | |||
|
|
|||
| - (void)appendLink:(id<BONTextable>)link separator:(NSString *)separator | |||
| { | |||
| if (separator.length > 0) { | |||
There was a problem hiding this comment.
#pp I think you can just forward this to appendLink:separatorTextable, and wrap separator in a BONChain. Might be problematic because you modified the tests instead of adding new tests, so they don't test this method any more. Not sure if it's worth keeping the old and new tests, though ¯_(ツ)_/¯
| @@ -441,4 +441,9 @@ - (BONChainColor)textColor | |||
| return self.color; | |||
| } | |||
|
|
|||
| - (void)appendLink:(id<BONTextable>)link separator:(NSString *)separator | |||
| { | |||
| [self appendLink:link separatorTextable:BONChain.new.string(separator)]; | |||
There was a problem hiding this comment.
#req I may have misled you before. I believe, to keep the original behavior, you need to use self.string(separator) here. If you change it, you'll see that some of your test changes result in failing tests.
ZevEisenberg
deleted the
feature/connorneville/Chain-Concatenation-Textable-Separator
branch
Fixes #160