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

Rename "withCollectionView:" param to "collectionView:" #491

Closed
wants to merge 6 commits into from
Closed

Rename "withCollectionView:" param to "collectionView:" #491

wants to merge 6 commits into from

Conversation

vincent-peng
Copy link

@vincent-peng vincent-peng commented Feb 14, 2017

  • Rename "withCollectionView:" param to "collectionView:" param to be consistent with the rest
    of the protocol.
  • Update tests.

Changes in this pull request

Related issue: #466

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

…apterUpdaterDelegate

- Rename `didPerformBatchUpdates` param to be consistent with the rest
of the protocol.
- Update tests.
@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!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

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

@vincent-peng vincent-peng changed the title Renaming "withCollectionView:" param to "collectionView:" in IGListAd… Rename "withCollectionView:" param to "collectionView:" Feb 14, 2017
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.

@vincent-peng awesome! Mind updating the CHANGELOG.md since this is a breaking change?

@jessesquires jessesquires added this to the 3.0.0 milestone Feb 14, 2017
@@ -39,7 +39,7 @@ NS_ASSUME_NONNULL_BEGIN
*/
- (void)listAdapterUpdater:(IGListAdapterUpdater *)listAdapterUpdater
didPerformBatchUpdates:(IGListBatchUpdateData *)updates
withCollectionView:(UICollectionView *)collectionView;
collectionView:(UICollectionView *)collectionView;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: formatting

@facebook-github-bot
Copy link
Contributor

@vincent-peng updated the pull request - view changes

CHANGELOG.md Outdated
- (void)listAdapterUpdater:(IGListAdapterUpdater *)listAdapterUpdater
didPerformBatchUpdates:(IGListBatchUpdateData *)updates
collectionView:(UICollectionView *)collectionView;
```
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We probably don't need the old/new here.

What about just saying

Renamed IGListAdapterUpdaterDelegate method to listAdapterUpdater:didPerformBatchUpdates:collectionView:

w/ the name + PR

@facebook-github-bot
Copy link
Contributor

@vincent-peng updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@vincent-peng updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@vincent-peng updated the pull request - view changes

@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