Issue 29 table view data source editing by psartzetakis · Pull Request #67 · jessesquires/JSQDataSourcesKit

psartzetakis · 2016-09-14T17:13:36Z

Pull request checklist

All tests pass. Demo project builds and runs.
I have resolved any merge conflicts.
I have followed the coding style, and reviewed the contributing guidelines.

This fixes issue #29 .

What's in this pull request?

The edit functionality of tableView provided by the UITableViewDataSource is now exposed through JSQDataSourcesKit

Example mixed swift3.0. close #59.

# Conflicts: # Source/BridgedDataSource.swift # Source/BridgedFetchedResultsDelegate.swift

jessesquires · 2016-09-14T22:48:08Z

Thanks @psartzetakis ! 🎉

Just gave this a glance for now. Looks good! I'll give more feedback soon.

jessesquires · 2016-09-17T09:49:57Z

Hey @psartzetakis !

Just merged swift3.0 into develop. Do you mind updating this to merge into develop instead?

psartzetakis · 2016-09-17T11:26:37Z

Hey @jessesquires ,

Just changed it to develop 👍

…leViewDataSource_editing # Conflicts: # .travis.yml # Source/BridgedFetchedResultsDelegate.swift # Source/DataSourceProvider.swift

codecov-io · 2016-09-17T17:19:41Z

Current coverage is 93.59% (diff: 96.77%)

Merging #67 into develop will increase coverage by 0.26%

@@            develop        #67   diff @@
==========================================
  Files             9         10     +1   
  Lines           360        390    +30   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            336        365    +29   
- Misses           24         25     +1   
  Partials          0          0

Diff Coverage	File Path
•••••••• 83%	Source/BridgedDataSource.swift
•••••••••• 100%	new Source/TableDataSourceEditingController.swift
•••••••••• 100%	Source/DataSourceProvider.swift
•••••••••• 100%	Source/DataSource.swift

Powered by Codecov. Last update d86929f...97f94dc

…leViewDataSource_editing ViewDataSource_editing # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

…ting' into issue_29_tableViewDataSource_editing

psartzetakis · 2016-10-13T14:25:12Z

Hi @jessesquires ,
I was wondering if there is anything that I have a missed or anything else I should do?

jessesquires

Thanks so much @psartzetakis !

Left some review comments, mostly just some style nits. 😄

jessesquires · 2016-10-28T15:44:06Z

            return cell
        }
+
+        //3. Optional - create if neccessary a datasourceEditingController to enable the editing functionality on the tableView


nit: space after //

jessesquires · 2016-10-28T15:45:12Z

 internal typealias TableTitleForFooterInSectionHandler = (Int) -> String?

+internal typealias TableCanEditRowAtIndexPathHandler = (UITableView, IndexPath) -> Bool
+internal typealias TableCommitEditingStyleForRowAtIndexPathHandler = (UITableView, UITableViewCellEditingStyle, IndexPath) -> Void


nit: let's have shorter names here:

TableCanEditHandler

TableCommitEditingStyleHandler

jessesquires · 2016-10-28T15:46:50Z

    var tableTitleForFooterInSection: TableTitleForFooterInSectionHandler?
+
+    var tableCanEditRowAtIndexPath: TableCanEditRowAtIndexPathHandler?
+    var tableCommitEditingStyleForRowAtIndexPath: TableCommitEditingStyleForRowAtIndexPathHandler?


nit, same as above:

var tableCanEditRow

var tableCommitEditingForRow

jessesquires · 2016-10-28T15:47:39Z

    }
+
+    @objc func tableView(_ tableView: UITableView, canEditRowAt indexPath: IndexPath) -> Bool {
+        if let closure = tableCanEditRowAtIndexPath{


nit: space before {

jessesquires · 2016-10-28T15:49:05Z

+     Removes an Item at a specific `IndexPath`
+
+     - parameter indexPath: The index path specifying the location of the cell
+     - returns: The item specified by indexPath, or `nil`


nit: need a . after each sentence 😄

jessesquires · 2016-10-28T16:11:01Z

+        return canEditConfigurator(indexPath, tableView)
+    }
+
+    public func configureCommitEditStyleForRow(in tableView: UITableView, editingStyle: UITableViewCellEditingStyle, at indexPath: IndexPath) {


nit: remove "configure"

jessesquires · 2016-10-28T16:11:38Z

+        })
+    }
+
 }


jessesquires · 2016-10-28T16:11:49Z

        let item = dataSource[ip]

-        // THEN: we receive the exepected data
+        // THEN: we receive the expected data


jessesquires · 2016-10-28T16:12:15Z

+        // THEN: the removedItem is the expected item
+        let removedItem = dataSource.remove(at: ip)
+        XCTAssertEqual(removedItem, itemToRemove)
+    }


we should test out-of-bounds indexPaths, too 😄

jessesquires · 2016-10-28T16:14:02Z


-
+    //The data source that provides the editing functionality on the tableView
+    public var dataSourceTableEditing: DataSourceTableEditingProtocol?


it would be nice to only have this property for tableView dataSources... but I'm not sure if we can express that?

perhaps:

this could be private (or fileprivate)

then, we could expose the setter for only tableView dataSources

?

I believe we can add that requirement here
public extension DataSourceProvider where CellFactory.View: UITableViewCell.
So they will be able to set the property of tableEditingController
what do you think?
Sorry ignore me for now, having second thoughts... :)

jessesquires · 2016-10-28T16:15:43Z

One more thing: I updated this branch via the GitHub UI, so you might need to pull.

Oh, and again, sorry for the delay here! 😊

jessesquires · 2016-10-28T16:34:26Z

I think the biggest thing here is figuring out how to expose the TableEditingController to only tableView.

cc @dcaunt - any thoughts?

psartzetakis · 2016-11-09T15:11:12Z

Hi @jessesquires ,
Just pushed some changes :)

jessesquires

Thanks so much @psartzetakis ! 🎉

So sorry it took me so long to review here. Overall this looks great! 👍 Just a few more minor comments.

If you don't have time, we carry these changes forward for you. Just let us know.

jessesquires · 2016-12-11T18:52:49Z

-
+
+    //The data source that provides the editing functionality on the tableView
+    fileprivate var dataSourceTableEditing: TableDataSourceEditingController?


why not make this public and remove setTableDataSourceEditingController(_:) below?

also, we can remove the comment 😄

Hi @jessesquires ,
Thanks for the review 😄 . The reason that I didn't make it public in the first place is because I wanted to constraint it (through the extension) so that it can be used only for a tableView. Since I cannot have a stored property in an extension I thought of setting the dataSourceTableEditing through a public function in the extension. Another option would be to have a public computed property in the extension that it's setter/getter will use either objc runtime or they will access directly the fileprivate var dataSourceTableEditing. Please let me know your thoughts on that. There might be a way that I have missed or we might don't want any of these approaches 😄

After we sort this I will proceed with applying the changes 👍

oh, duh! sorry 😄

Ok, this looks good structurally. Only suggestion then is to follow the style of
public var tableViewDataSource: UITableViewDataSource

let's have a fileprivate var _tableEditingController and a public var tableEditingController (computed) in the extension 👍

jessesquires · 2016-12-11T18:53:19Z

+
+        dataSource.tableCommitEditingStyleForRow = { [unowned self] (tableView, editingStyle,indexPath) in
+            self.dataSourceTableEditing?.commitEditStyleForRow(in: tableView, editingStyle: editingStyle, at: indexPath)
+        }


jessesquires · 2016-12-11T18:54:38Z

+//
+//  Created by Panagiotis Sartzetakis on 12/09/2016.
+//  Copyright © 2016 Hexed Bits. All rights reserved.
+//


let's replace with the project comment headers 😄

can we rename this file to TableDataSourceEditingController.swift ?

jessesquires · 2016-12-11T18:55:24Z

+        return commitEditingStyle(tableView, editingStyle, indexPath)
+    }
+
+}


jessesquires · 2016-12-11T19:05:16Z

+        let row = indexPath.row
+
+        return row < numberOfItems(inSection: section)
+    }


I think we can get rid of both of these.

Instead, we can just check if dataSource.item(atIndexPath: ip) == nil

ok cool, I know that you have opened a PR #76 for making public the function dataSource.item(atIndexPath: ip) but I need to make it public as well in order to use it ..😄

jessesquires · 2016-12-11T19:07:01Z

+        let section = indexPath.section
+        let row = indexPath.row
+        return sections[section].items.remove(at: row)
+    }


jessesquires · 2016-12-11T19:08:50Z

+     Removes an Item at a specific `IndexPath`.
+
+     - parameter indexPath: The index path specifying the location of the cell.
+     - returns: The item specified by indexPath, or `nil`.


Let's update to:

/** Removes an item at the specified index path. - parameter indexPath: The index path specifying the location of the item. - returns: The item at `indexPath`, or `nil` if it does not exist.

jessesquires · 2016-12-11T19:11:56Z

+
+        dataSourceProvider?.setTableDataSourceEditingController(tableDataSourceEditingController)
+
+        // 5. set data source


…leViewDataSource_editing

jessesquires · 2016-12-13T01:17:19Z

Awesome 🎉

jessesquires · 2016-12-13T01:18:38Z

     - parameter dataSource:           The data source.
     - parameter cellFactory:          The cell factory.
     - parameter supplementaryFactory: The supplementary view factory.
+     to edit it's cells


jessesquires · 2016-12-13T16:43:08Z

Thanks so much @psartzetakis ! 🎉

- refine closure param order - rename TableEditingController - refine public interface to TableEditingController - refine naming - update example app

jessesquires · 2016-12-13T17:33:24Z

@psartzetakis

We really appreciate the help and effort with this PR! It was great. I've sent you an invite to be a collaborator on this project. This will give you push access + other permissions. 😄

There's no pressure to accept or even contribute anything else if you don't want to! 😊

psartzetakis · 2016-12-14T09:07:22Z

Really glad I helped @jessesquires 🎉 . Thanks for the invitation, it will be a pleasure to keep contributing. 😄

jessesquires and others added 18 commits July 17, 2016 11:02

update to swift 2.3, #53

55e9a0c

update travis for swift 2.3, #53

56af816

initial migration to swift 3.0, #54

f6029a2

update xcode 8 beta 3, #54

2f442d4

Merge branch 'develop' into swift2.3

77540a7

proj settings

63ac07f

Merge branch 'swift2.3' into swift3.0

4912ecc

Update mixed cell type example for Swift 3.0

4ee1a40

Merge pull request #60 from dcaunt/example-mixed-swift3.0

8d680f1

Example mixed swift3.0. close #59.

minor demo updates

5be49cf

update Xcode 8 beta 4, #54

04cb5de

Merge branch 'develop' into swift2.3

69bb4e2

Merge branch 'swift2.3' into swift3.0

fbba40d

update for Xcode8 beta6, #54

96c97c5

update for xcode8 GM, #54

5e12e27

name closure params

a51bfe6

edit functionality support for UITableViewDataSource

386e5dc

Merge branch 'swift3.0' into issue_29_tableViewDataSource_editing

923f174

# Conflicts: # Source/BridgedDataSource.swift # Source/BridgedFetchedResultsDelegate.swift

jessesquires added the enhancement label Sep 14, 2016

jessesquires force-pushed the swift3.0 branch 2 times, most recently from e31c1fe to 2a31162 Compare September 17, 2016 09:38

psartzetakis changed the base branch from swift3.0 to develop September 17, 2016 11:25

Merge remote-tracking branch 'jessesquires/develop' into issue_29_tab…

22264e8

…leViewDataSource_editing # Conflicts: # .travis.yml # Source/BridgedFetchedResultsDelegate.swift # Source/DataSourceProvider.swift

Panagiotis Sartzetakis added 3 commits September 18, 2016 10:42

Merge branch 'develop' into issue_29_tableViewDataSource_editing

78de1c1

Merge remote-tracking branch 'origin/issue_29_tableViewDataSource_edi…

311488a

…ting' into issue_29_tableViewDataSource_editing

jessesquires self-assigned this Oct 1, 2016

Merge branch 'develop' into issue_29_tableViewDataSource_editing

d98fc67

jessesquires suggested changes Oct 28, 2016

View reviewed changes

Merge branch 'develop' into issue_29_tableViewDataSource_editing

356f15e

minor updates

250783f

jessesquires suggested changes Dec 11, 2016

View reviewed changes

Panagiotis Sartzetakis added 2 commits December 12, 2016 16:46

Merge remote-tracking branch 'jessesquires/develop' into issue_29_tab…

b94065b

…leViewDataSource_editing

review updates

c62032a

Merge branch 'develop' into issue_29_tableViewDataSource_editing

7305f83

jessesquires reviewed Dec 13, 2016

View reviewed changes

minor update

97f94dc

jessesquires approved these changes Dec 13, 2016

View reviewed changes

jessesquires merged commit a9a90b0 into jessesquires:develop Dec 13, 2016

This was referenced Dec 13, 2016

Support additional UITableViewDataSource methods #29

Closed

Add docs for TableEditingController #78

Closed

jessesquires added a commit that referenced this pull request Dec 13, 2016

follow-ups / clean-up from #67

a454719

- refine closure param order - rename TableEditingController - refine public interface to TableEditingController - refine naming - update example app

This was referenced Dec 13, 2016

Implement data source insert convenience methods #79

Closed

Refine TableEditingController design #80

Closed

Update changelog for TableEditingController (#67) #82

Closed

psartzetakis deleted the issue_29_tableViewDataSource_editing branch December 14, 2016 11:53

jessesquires modified the milestones: 6.1.0, 7.0.0 Nov 6, 2017

+                      })
+                  }
               }



		//The data source that provides the editing functionality on the tableView
		public var dataSourceTableEditing: DataSourceTableEditingProtocol?



		//The data source that provides the editing functionality on the tableView
		fileprivate var dataSourceTableEditing: TableDataSourceEditingController?


		dataSourceProvider?.setTableDataSourceEditingController(tableDataSourceEditingController)

		// 5. set data source

Uh oh!

Conversation

psartzetakis commented Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

This fixes issue #29 .

What's in this pull request?

Uh oh!

jessesquires commented Sep 14, 2016

Uh oh!

jessesquires commented Sep 17, 2016

Uh oh!

psartzetakis commented Sep 17, 2016

Uh oh!

codecov-io commented Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 93.59% (diff: 96.77%)

Uh oh!

psartzetakis commented Oct 13, 2016

Uh oh!

jessesquires left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psartzetakis Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessesquires commented Oct 28, 2016

Uh oh!

jessesquires commented Oct 28, 2016

Uh oh!

psartzetakis commented Nov 9, 2016

Uh oh!

jessesquires left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psartzetakis commented Sep 14, 2016 •

edited

Loading

codecov-io commented Sep 17, 2016 •

edited

Loading

psartzetakis Nov 9, 2016 •

edited

Loading