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

Handle nil section controllers from data source #488

Closed
wants to merge 2 commits into from
Closed

Conversation

rnystrom
Copy link
Contributor

Changes in this pull request

Removing the assert and handling nil section controllers. This wont effect Swift code which demands a non-optional section controller, but Objective-C is more nuanced. There is evidence that some callers do not have a default state and are returning nil.

Unfortunately this will OOB here if using the original objects array provided by the data source. Instead we prune the array, only allowing in objects that have a matching section controller.

Fixes t15773862 internally.

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.

Sorry, something went wrong.

@rnystrom rnystrom added the bug label Feb 13, 2017
@rnystrom rnystrom added this to the 3.0.0 milestone Feb 13, 2017
@facebook-github-bot
Copy link
Contributor

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

@jessesquires
Copy link
Contributor

jessesquires commented Feb 13, 2017

@rnystrom -- But allowing nil here contradicts the API contract, which specifies that implementers must return non-nil section controllers.

Shouldn't we just handle the OOB error instead?
https://github.com/Instagram/IGListKit/blob/master/Source/Internal/IGListSectionMap.m#L63

[objects enumerateObjectsUsingBlock:^(id object, NSUInteger idx, BOOL *stop) {
        if (idx >= sectionControllers.count) {
              // log warning to console?
              continue;
        }

        IGListSectionController<IGListSectionType> *sectionController = sectionControllers[idx];
        
        // ....
}];

@rnystrom
Copy link
Contributor Author

@jessesquires and I just chatted offline:

  • This diff fixes the OOB by just skipping the object+section controller if the SC is nil
  • Keeping the assert makes testing difficult. We could add a check NSStringFromClass(@"XCTestCase") == nil before asserting, but that's kind of gross

I'm going to add a warning log if the SC is nil so we can cleanly test and at least raise some complaints.

@facebook-github-bot
Copy link
Contributor

@rnystrom updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

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

@jessesquires jessesquires deleted the update-crash branch February 14, 2017 19:54
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.

None yet

3 participants