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

Added Subspec for Diffing #368

Closed
wants to merge 13 commits into from
Closed

Conversation

Sherlouk
Copy link
Contributor

@Sherlouk Sherlouk commented Dec 24, 2016

Changes in this pull request

So the main Podspec file now has two Subspecs, Diffing and for lack of better name Default. Diffing is purely the files within the common directories (used for Diffing, if in the future we support MacOS NSCollectionView then this may need changing). Default depends on Diffing but also adds all the non-common files.

To use it via CocoaPods, nothing changes. You import IGListKit and you will only have access to the files in the pod you use. (So if you use IGListKit/Diffing in your Podfile, then you will only get access to the diffing files). If you do a manual installation, or I assume via Carthage, then you will need to import the correct header file (either IGListDiffKit.h or IGListKit.h depending on what you want).

Turns out CocoaPods creates it's own umbrella header (unless you tell it otherwise) meaning our ones are unused by it.

Just to confirm IGListKit in your Podfile will give you everything, only if you add /Diffing will you "opt-out" of the extra functionality. MacOS users can use which ever one they choose, and will get the same functionality (at this point in time)

Do I need to update changelog for this?
Leaving myself a few TODOs once we've discussed this

Pull request checklist

  • All tests pass. Demo project builds and runs.
  • I added tests, an experiment, or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
  • I have reviewed the contributing guide

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling db8f9f4 on Sherlouk:podspec into c6b65cf on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 8ceff68 on Sherlouk:podspec into c6b65cf on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@coveralls
Copy link

coveralls commented Dec 24, 2016

Coverage Status

Coverage remained the same at 97.978% when pulling 2f90281 on Sherlouk:podspec into c6b65cf on Instagram:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 2f90281 on Sherlouk:podspec into c6b65cf on Instagram:master.

@jessesquires
Copy link
Contributor

@Sherlouk

Copy link
Contributor

@jessesquires jessesquires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few follow-ups here:

  1. We'll need to merge Added compatibility header #369 first and rebase this.
  2. Definitely need to note this in the changelog
  3. Let's add Guides/Installation.md to explain and let's link to it at the end of the "Installation" section in the README. ("For advanced usage, see our Installation Guide.")

@jessesquires
Copy link
Contributor

Oh, one last thing:

We'll probably need to strip the last commit here (2f90281).

Our importing system chokes on .lock and .xcconfig files. That might break travis, but we'll fixup and run pod install internally before merging.

@jessesquires
Copy link
Contributor

One more 😊

I noticed you needed to move IGListKit.h -- we'll need to do that internally too.

I'll do that when I merge #369, so your rebase here is easier.

@jessesquires jessesquires modified the milestone: 2.1.0 Dec 24, 2016
@Sherlouk
Copy link
Contributor Author

@jessesquires Superb!

I'll revert the last commit and then update documentation, merge 369 whenever and I'll rebase!

Obviously not aware how all your internal stuff works, so apologies for that

@jessesquires
Copy link
Contributor

Obviously not aware how all your internal stuff works

Me either 🤷‍♂️ 😆 I just know it breaks all the time. 😊

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

1 similar comment
@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@Sherlouk
Copy link
Contributor Author

Build will continue to fail now until it has been fully merged as:
1 - I have links to pages that don't yet exist (Advanced Installation guide via the HTML page on Jazzy)
2 - I have reverted the pod install

# Conflicts:
#	CHANGELOG.md
#	IGListKit.xcodeproj/project.pbxproj
@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@Sherlouk
Copy link
Contributor Author

@jessesquires

All rebased & conflict free!
Changelog updated, Guide added
Tests are working fine locally
Haven't run pod install as requested

Do you want me to update docs, or would you rather do that internally?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 00d2852 on Sherlouk:podspec into 19d4f9d on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@Sherlouk
Copy link
Contributor Author

@jessesquires All good!

Interesting bug with the facebook bot though, thinks all the changes were made by me!

@jessesquires
Copy link
Contributor

We'll do the pod install and docs internally.

#import <UIKit/UIKit.h>
#else
#import <Cocoa/Cocoa.h>
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import the new compatibility header?

@Sherlouk
Copy link
Contributor Author

Will update to use compatibility header, also README references wrong file name since rename

@jessesquires
Copy link
Contributor

  • One minor inline comment to fix.

I'll leave your move of IGListKit.h and resolve stuff on our side internally after importing.

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 138c1e7 on Sherlouk:podspec into 19d4f9d on Instagram:master.

@Sherlouk
Copy link
Contributor Author

@jessesquires This also closes #53?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 138c1e7 on Sherlouk:podspec into 19d4f9d on Instagram:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 138c1e7 on Sherlouk:podspec into 19d4f9d on Instagram:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 138c1e7 on Sherlouk:podspec into 19d4f9d on Instagram:master.

'Source/Common/Internal/*.h'
]
s.subspec 'Default' do |cs|
cs.dependency 'IGListKit/Diffing'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slick!


```ogdl
github "Instagram/IGListKit" ~> 2.0.0
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can followup w/ manual install steps too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did have that, but @jessesquires removed it -- not sure reasoning behind that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, sorry.

Removed because manual installation doesn't really work -- it's not just "drag and drop" because our test target depends on OCMock via CocoaPods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth just leaving it out then, I'm sure if you really want to manually install it then you'll be able to work it out -- or ask in the issues


```ogdl
github "Instagram/IGListKit" ~> 2.0.0
```

> **For advanced usage, see our [Installation Guide](https://instagram.github.io/IGListKit/installation.html)**.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we nix the bold?

*/
FOUNDATION_EXPORT const unsigned char IGListKitVersionString[];

// Common (iOS, tvOS, macOS compatible):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we remove this?

@@ -17,6 +17,8 @@ This release closes the [2.1.0 milestone](https://github.com/Instagram/IGListKit

- Working ranges now work with `IGListStackedSectionController`. [Ryan Nystrom](https://github.com/rnystrom) [(#356)](https://github.com/Instagram/IGListKit/pull/356)

- Added CocoaPods subspec for diffing, `IGListKit/Diffing` and an [installation guide](https://instagram.github.io/IGListKit/installation.html). [Sherlouk](https://github.com/Sherlouk) [(#368)](https://github.com/Instagram/IGListKit/pull/368)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥🔥🔥

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 2836ebd on Sherlouk:podspec into 19d4f9d on Instagram:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 2836ebd on Sherlouk:podspec into 19d4f9d on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@jessesquires has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@jessesquires has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pod subspec for 'IGListKit/Diffing' Make diff code available as a separate component
5 participants