Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for storing a single item #2

Merged
merged 10 commits into from May 15, 2019
Merged

Add support for storing a single item #2

merged 10 commits into from May 15, 2019

Conversation

@JosephDuffy
Copy link
Contributor

JosephDuffy commented Apr 12, 2019

I'm not 100% about the lack of support for optionals here.

On one hand I think it could be up to the caller to not create this with a nil element, but on the other hand I can see it might be useful to return 0 for numberOfSections and numberOfElements(in:) when the value is nil 馃

@JosephDuffy JosephDuffy requested a review from shaps80 Apr 12, 2019
JosephDuffy and others added 3 commits Apr 12, 2019
Please remove the store, update header docs and test :)
Please remove the store, update header docs and test :)
self.element = element
}

public func numberOfElements(in section: Int) -> Int {

This comment has been minimized.

Copy link
@JosephDuffy

JosephDuffy Apr 26, 2019

Author Contributor

I actually just came up with a scenario where it's useful to have this return something other than 0 and 1 here. I will explain via PR in other project, but noting here and I think it'd be useful for this to be open

JosephDuffy added 2 commits Apr 26, 2019
- parameter section: An index number identifying the section. This index value is 0-based.
- returns: The number of elements in `section`
*/
open func numberOfElements(in section: Int) -> Int {

This comment has been minimized.

Copy link
@JosephDuffy

JosephDuffy Apr 26, 2019

Author Contributor

This is another occasion I think it would be better to separate the visual element vs the data element count. If I have a single model that may actually map to 2 cells on the screen it would be advantageous to return 1 here but 2 for something like numberOfVisualElements(in:)

This comment has been minimized.

Copy link
@shaps80

shaps80 Apr 26, 2019

Contributor

I'll give it some more thought.

public private(set) var element: Element

/// `true` if `element` is an `Optional.none`
public var elementIsNil: Bool {

This comment has been minimized.

Copy link
@shaps80

shaps80 May 3, 2019

Contributor

I don't agree with this being in the lib. Its an easy extension to add on later.
I don't see maybe users actually creating a SingleElementDataSource<Optional<String>> for example.
And this only makes sense in those context's.


/// The index paths this data source provides. Each `IndexPath` will
/// have a section of 0
public var indexPaths: [IndexPath] {

This comment has been minimized.

Copy link
@shaps80

shaps80 May 3, 2019

Contributor

Not sure the point of this and to be honest, this is a single element, it only ever has 1 indexPath which is always 0, 0
The API consumer doesn't have to use element(at indexPath:) in fact this dataSource doesn't even have that function. So they'd always just grab the element directly. The indexPath has very little value, but if they did want to have access to it, I'd imagine again its easier for the consumer to just add an extension:

var indexPath: IndexPath { return IndexPath(item: 0, section: 0) }
Copy link
Contributor

shaps80 left a comment

Just a few things to remove.

JosephDuffy added 3 commits May 9, 2019
# Conflicts:
#	Composed.xcodeproj/project.pbxproj
@JosephDuffy JosephDuffy requested a review from shaps80 May 9, 2019
@shaps80

This comment has been minimized.

Copy link
Contributor

shaps80 commented May 11, 2019

Looks great just one last thing I鈥檒l do it as a suggested change. Then I鈥檒l merge. Cheers dude

Sent with GitHawk

@shaps80 shaps80 merged commit 49eb7e2 into master May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can鈥檛 perform that action at this time.