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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unreducedEdits property #9

Closed
wants to merge 1 commit into from
Closed

Add unreducedEdits property #9

wants to merge 1 commit into from

Conversation

chrisamanse
Copy link

  • unreduced edits are edits without converting insertion/deletion pairs into .Move edits
  • unreduced edits is also good for manual changes in a UITableView or UICollectionView, since UITableView's only allow insertion, deletion, and update on rows.
  • converted tabs into spaces

- unreduced edits are edits without converting insertion/deletion pairs into ".Move" edits
- unreduced edits is also good for manual changes in a UITableView or UICollectionView, since UITableView's only allow insertion, deletion, and update on rows.
- converted tabs into spaces
@chrisamanse
Copy link
Author

I added a test case as well.

@osteslag
Copy link
Owner

Thanks for showing your interest in the project, @chrisamanse.

If I understand you correctly, you want to be able to get the “unreduced” edits on a Changeset, i.e., edits where .Deletion and .Insertion of the same element are not converted into a .Move, the reason being that neither UITableView nor UICollectionView supposedly supports moving cells…

Two comments:

  1. As a matter of fact, both view classes support moves. UITableView has moveRowAtIndexPath(_:toIndexPath:), UICollectionView also has moveRowAtIndexPath(_:toIndexPath:). You should use those.
  2. The feature you request could be implemented in a much less intrusive manner.

I may include the feature (but again, are you sure you even need it?). And if I do, it could be done in the following way, defaulting to the current behaviour:

// Add reduceMoves argument to initializer:
public init(source origin: T, target destination: T, reduceMoves: Bool = true) {
    // ...
    self.edits = Changeset.editDistance(source: self.origin, target: self.destination, reduceMoves: reduceMoves)
}

// Add same reduceMoves argument to editDistance function:
public static func editDistance(source s: T, target t: T, reduceMoves: Bool = true) -> [Edit<T.Generator.Element>] {
    // ...

    // Check flag before reducing:
    if reduceMoves == true {
        return reducedEdits(d[m][n])
    } else {
        return d[m][n]
    }
}

Less that 10 lines of change, compared to the +266/-160 you propose.

If you want to contribute to this project (and probably others as well), I encourage you to study the style and conventions of the existing code. Go back and look at pull requests, browse the discussions. (Quickly done on this project.)

Your pull request changes pretty much every line in the file, converting tabs to spaces. That not only makes it very difficult to see the effect of your changes, it also discards the project’s convention to use tab indentation. There is even a recent commit (0eb375e) which specifically enforces tabs. This project uses tabs.

Also, instead of making only one commit containing all of your changes at once, you should consider making several smaller commits, each containing an atomic change. So for example one commit would convert tabs to spaces, another contain the unreducedEdits property and function, and finally one adding the test. That would have made it easier for me understand your intentions, and I could potentially pick just some of your commits.

I will close this pull request.

@osteslag osteslag closed this Jan 19, 2016
@chrisamanse
Copy link
Author

Sorry about converting the tabs into spaces, I used a playground to test out the code, and simply copy and pasted the source file so it wasn't able to use the project's settings. I'll be mindful of my changes next time.

I've tried both the moveRowAtIndexPath(_:toIndexPath:) methods, and it worked. I got it confused with the UITableViewDataSource method tableView(_:moveRowAtIndexPath:toIndexPath:), where it is meant for observing movement caused by the user only. Thank you! And thanks for this useful framework.

@chrisamanse chrisamanse deleted the patch-1 branch January 21, 2016 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants