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

IGListAdapter: Fix not returning early when collectionView/dataSource is nil and completion is nil #51

Conversation

benasher44
Copy link
Contributor

@benasher44 benasher44 commented Oct 12, 2016

Changes in this pull request

  • I ran the static analyzer, and it found a case in reloadDataWithCompletion: where it looks like we meant to bail out early if the collectionView or dataSource is nil, but it only does so if completion is provided.
  • Fixed a spelling error in nearby documentation

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 have reviewed the contributing guide

Notes

  • There is one other analyzer warning, but it seems to be making an incorrect assumption. In any case, it's not something I can fix quickly (requires more time spent staring at IGListDiff.mm).
  • A ticket should be opened to add static analysis to .travis.yml. I'lll do this soonish, unless someone beats me to it!

Questions

  • I could add a test for this, but this project is annotated well enough with nullability annotations that it's easy for the analyzer to catch bugs like this (it could tell there was a case where the nil collectionView might make it somewhere it shouldn't have). Should I add a test anyway?
  • I had to add the IGListKitTests scheme manually to run the tests. Should I make it a shared scheme and commit it?

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@benasher44
Copy link
Contributor Author

Attempting to fix the spurious CocoaPods failures in #57

@@ -103,14 +103,14 @@ IGLK_SUBCLASSING_RESTRICTED
- (void)performUpdatesAnimated:(BOOL)animated completion:(nullable IGListUpdaterCompletion)completion;

/**
Perform an immediate reload of the data in the data source, discarding the old objectss.
Copy link
Contributor

Choose a reason for hiding this comment

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

😂 nice one

@rnystrom
Copy link
Contributor

@benasher44 thanks for fixing the builds! We had a scramble to get all the public CI stuff working yesterday 😅

Awesome find here!

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are a Facebook employee, you can view this diff on Phabricator.

@benasher44
Copy link
Contributor Author

Np!

@ocrickard
Copy link

lol, nice find!

@jessesquires
Copy link
Contributor

👏 🙌

@jessesquires jessesquires modified the milestone: 2.0.0 Oct 12, 2016
@jessesquires jessesquires mentioned this pull request Oct 12, 2016
benasher44 added a commit to benasher44/IGListKit that referenced this pull request Oct 13, 2016
facebook-github-bot pushed a commit that referenced this pull request Oct 13, 2016
Summary:
Fixed #63. I used the style we use in CocoaPods, but I'm happy to adjust!

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/CONTRIBUTING.md)
Closes #67

Reviewed By: jessesquires

Differential Revision: D4015999

Pulled By: rnystrom

fbshipit-source-id: bbe8055f22e84c5bdc628b4c1d95dab111774e12
facebook-github-bot pushed a commit that referenced this pull request Oct 14, 2016
Summary:
- Travis appears to be using CocoaPods 1.1.0.beta.2, which is missing some fixes for Xcode 8 (see [sample failing build](https://travis-ci.org/Instagram/IGListKit/jobs/166935850) from #51).
- This change will ensure that a consistent CocoaPods version is used by Travis
- In the added Gemfile, I picked the latest CocoaPods 1.1.0 RC (matches version in Podfile.lock) and the latest xcpretty.
- Changed `pod spec lint` to `pod lib lint` to verify local files instead of files from the version specified in the spec.

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/CONTRIBUTING.md)
Closes #57

Differential Revision: D4019655

Pulled By: rnystrom

fbshipit-source-id: 422e55c44dfdf276b587ea6e12ae30218a237ff5
bogren pushed a commit to bogren/IGListKit that referenced this pull request Aug 3, 2017
Summary:
- Travis appears to be using CocoaPods 1.1.0.beta.2, which is missing some fixes for Xcode 8 (see [sample failing build](https://travis-ci.org/Instagram/IGListKit/jobs/166935850) from Instagram#51).
- This change will ensure that a consistent CocoaPods version is used by Travis
- In the added Gemfile, I picked the latest CocoaPods 1.1.0 RC (matches version in Podfile.lock) and the latest xcpretty.
- Changed `pod spec lint` to `pod lib lint` to verify local files instead of files from the version specified in the spec.

- [x] All tests pass. Demo project builds and runs.
- [x] I added tests, an experiment, or detailed why my change isn't tested.
- [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/CONTRIBUTING.md)
Closes Instagram#57

Differential Revision: D4019655

Pulled By: rnystrom

fbshipit-source-id: 422e55c44dfdf276b587ea6e12ae30218a237ff5
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.

5 participants