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 -[IGListAdapter objectForSectionController:] helper method -- iss… #204

Closed
wants to merge 1 commit into from

Conversation

saraswatayu
Copy link
Contributor

@saraswatayu saraswatayu commented Nov 16, 2016

Changes in this pull request

Add -[IGListAdapter objectForSectionController:] helper method

Fixes #201

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

Copy link
Contributor

@rnystrom rnystrom left a comment

Choose a reason for hiding this comment

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

Small update on the assert, and it looks like it might need a rebase (some conflicts).

Thanks for knocking this out so quickly! Issue wasn't even open for an hour 🙏

adjustForUpdateBlock:NO];
NSArray *paths0 = [self.adapter indexPathsFromSectionController:second
indexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(2, 4)]
adjustForUpdateBlock:NO];
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

IGAssertMainThread();
IGParameterAssert(sectionController != nil);

NSUInteger section = [self.sectionMap sectionForSectionController:sectionController];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: const

IGParameterAssert(sectionController != nil);

NSUInteger section = [self.sectionMap sectionForSectionController:sectionController];
IGAssert(section != NSNotFound, @"Section nil for section controller %@", sectionController);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm if this assert will be too aggressive. Sometimes weird things happen and its probably ok to return nil in that case (for instance, the -[IGListSectionMap objectForSection:] method actually returns nil if the section is OOB).

I think we should probably omit this assert.

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 based this off the implementation for -[IGListAdapter objectAtSection:], which crashes if you pass in an index that's out of bounds. The assert I wrote actually wouldn't work, but there definitely needs to be a bounds check.

I'm assuming that's another issue that should be fixed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@saraswatayu that method should have been updated (source here) so it doesn't assert.

Are you looking at the stable branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh I see your commit on the SectionMap! I didn't realize my earlier rebase to my fork didn't quite work as expected; I'll update this real quick.

@jessesquires jessesquires added this to the 2.0.0 milestone Nov 16, 2016
@jessesquires
Copy link
Contributor

@saraswatayu -- Please add this to CHANGELOG.md too 😄

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.

Let's some doc nits, too 😊

@@ -137,6 +137,15 @@ IGLK_SUBCLASSING_RESTRICTED
- (__kindof IGListSectionController <IGListSectionType> * _Nullable)sectionControllerForObject:(id)object;

/**
Fetch the object corresponding to a list in the feed. Constant time lookup.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, let's say:

Returns the object corresponding to the specified section controller in the list. Constant time lookup.

@@ -137,6 +137,15 @@ IGLK_SUBCLASSING_RESTRICTED
- (__kindof IGListSectionController <IGListSectionType> * _Nullable)sectionControllerForObject:(id)object;

/**
Fetch the object corresponding to a list in the feed. Constant time lookup.

@param sectionController A list object.
Copy link
Contributor

Choose a reason for hiding this comment

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

@param sectionController A section controller in the list.


@param sectionController A list object.

@return An object or nil.
Copy link
Contributor

@jessesquires jessesquires Nov 16, 2016

Choose a reason for hiding this comment

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

@return The object for the specified section controller, or nil if not found.

@facebook-github-bot
Copy link
Contributor

@saraswatayu updated the pull request - view changes

@saraswatayu
Copy link
Contributor Author

@jessesquires @rnystrom Thanks for your comments! It should now be updated - let me know if there's something else that catches your eye. 😄

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 91.465% when pulling e912565 on saraswatayu:ayush-201 into 204f7d5 on Instagram:master.

@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.

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