DiffCalculator for NSTableView

ChrisBeeson commented 8 years ago

Hey,

I would like to refactor your current iOS offerings to include OSX support - NSTableView and perhaps NSCollectionView.

Would that be ok? And if so, have you any preference on how you would like it done?

Chris

jflinter commented 8 years ago

Hi there! I'd love to merge a PR that contained this. I'd just ask a few things of it:

It should be contained in a separate file (probably Dwifft+AppKit.swift) and compiled only for OSX
It should have strong unit test coverage (see the tests for TableViewDiffCalculator)
It follows roughly the same code structure and formatting as the rest of Dwifft (I'm not really an OSX developer, so the closer it is to what exists today, the easier it'll be for me to understand and maintain in the future)

Thanks so much! Jack

DivineDominion commented 7 years ago

@ChrisBeeson any progress on this front?

rikchilvers commented 7 years ago

I've started work on this here on a branch based off the swift4 one. I've yet to add any test coverage (waiting for typelift/SwiftCheck to be updated to Swift 4 and Xcode 9) but the CollectionViewDiffCalculator works in my app.

@jflinter I've put the macOS stuff in a new scheme like the tvOS one. When I get to it, I'll put the corresponding tests in their own file, even though they'll closely mirror the iOS ones. Anything else you'd like me to change before pulling in the changes?

jack-stripe commented 7 years ago

@rikchilvers that plan sounds great - let me know when you've got something ready for me to take a look at. Thanks! (And, sorry for the slow reply to your note.)

rikchilvers commented 7 years ago

@jack-stripe Tests are done and passing!

Some things you might like to check:

The macOS version hides the SectionedTableViewDiffCalculator from the API in favour of a renamed (SingleSection)TableViewCalculator since NSTableViews don't support sections.
I couldn't get carthage to get the SwiftCheck version you forked so I had to move to the master
I've renamed the macOS framework so it builds as 'Dwifft.framework' too, rather than 'Dwifft-macOS.framework', which I noticed the SwiftCheck folks also do.

jack-stripe commented 7 years ago

Hey @rikchilvers, Sorry again for the slow response time; I haven't really had time to look at Dwifft lately, although I'm going to carve out time to try and get this branch merged.

Would you be able to do 2 things:

Open a PR for these changes, making sure to give me the ability to make changes to the branch. Will improve my time to communication.
Scaffold out a macOS example application? I don't think we should ship this without one.

Thanks again for your help and patience.

rikchilvers commented 7 years ago

No worries. I’ve submitted a pull request including a sample macOS app. I don’t have any familiarity with Travis so I’ll have to let you update that.

On 30 Oct 2017, at 23:35, Jack Flintermann notifications@github.com wrote:

Hey @rikchilvers https://github.com/rikchilvers, Sorry again for the slow response time; I haven't really had time to look at Dwifft lately, although I'm going to carve out time to try and get this branch merged.

Would you be able to do 2 things:

Open a PR for these changes, making sure to give me the ability to make changes to the branch. Will improve my time to communication. Scaffold out a macOS example application? I don't think we should ship this without one. Thanks again for your help and patience.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jflinter/Dwifft/issues/14#issuecomment-340616121, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnbomvPPreZyvYZqG15VXINy1NnFLxks5sxl0qgaJpZM4HufXP.

jflinter commented 6 years ago

Super happy to finally close this! Thanks @rikchilvers.

jflinter / Dwifft

DiffCalculator for NSTableView #14