Rename all references from ViewModel to Item #20
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
onmyway133
reviewed
Sep 26, 2016
| describe("ViewModel") { | ||
| var viewModel: ViewModel! | ||
| describe("Item") { | ||
| var Item: Item! |
There was a problem hiding this comment.
I think the variable name should be lowercase 😉
There was a problem hiding this comment.
I's weird that it compiles 😱
There was a problem hiding this comment.
I'm gonna check it :)
There was a problem hiding this comment.
@vadymmarkov it didn't compile when running the tests :)
There was a problem hiding this comment.
Maybe it should build the tests when calling build instead of just when the tests run. That would avoid these kind of things.
|
@vadymmarkov @onmyway133 @RamonGilabert yay, the tests pass! |
onmyway133
reviewed
Sep 26, 2016
| @@ -2,7 +2,7 @@ import Tailor | |||
|
|
|||
| // MARK: - Array | |||
|
|
|||
| public extension _ArrayType where Generator.Element == ViewModel { | |||
| public extension _ArrayType where Generator.Element == Item { | |||
There was a problem hiding this comment.
Does making extension on slightly private type good practice?
onmyway133
reviewed
Sep 26, 2016
| } | ||
|
|
||
| it("compare relations properly") { | ||
| var Item2 = Item(data) |
There was a problem hiding this comment.
Small detail, but it would be great if the variable is lowercase 😄
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a breaking change, it renames the
ViewModelstruct toItemandViewConfigurabletoItemConfigurable.It should be very easy to migrate to the new version as you only need to search and replace those changes and should be done in a jiffy.
This PR came to be after a discussion that me and @vadymmarkov had about naming.
ViewModelkind of implies that it holds content for the entire screen as it would potentially deal withViewController.And the second reason is that it is mainly used in
Spotswhere we always refer to theViewModelas anItem.Itemis abstract to work on multiple instances, where asViewModelimplies a specific scenario where it will be used. You can always argue that it is in fact styling a specific view asViewConfigurableis a property on the underlaying component, a cell for example.Personally I think this is the best name for what it does. I hope you do as well.