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.

Sign up for GitHub

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

Jump to bottom

Add missed delegate call #trivial #1211

Closed
wants to merge 3 commits into from
Closed

Add missed delegate call #trivial #1211

wants to merge 3 commits into from

Conversation

zhubofei
Copy link

@zhubofei zhubofei commented Jul 9, 2018

Changes in this pull request

Issue fixed: #

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

@zhubofei zhubofei requested a review from rnystrom July 9, 2018 04:45
@jessesquires jessesquires added this to the 4.0.0 milestone Jul 9, 2018
@jessesquires
Copy link
Contributor

Was this shipped in 3.x versions? If so, we need a changelog entry

rnystrom
rnystrom suggested changes Jul 9, 2018
View reviewed changes
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.

+1 changelog. Also could we add a failing test that ends up passing?

@zhubofei
Copy link
Author

zhubofei commented Jul 9, 2018 •
edited
Loading

@rnystrom Could you give me some detail about the test? I just checked Coveralls and noticed that all the delegate calls are uncovered, so there is no example I can follow.
screen shot 2018-07-09 at 5 43 27 pm

@zhubofei zhubofei closed this Jul 13, 2018
@zhubofei zhubofei reopened this Jul 13, 2018
@rnystrom
Copy link
Contributor

@zhubofei this would be a great opportunity to add more coverage to IGListKit! We usually test delegates w/ OCMock.

  • Create a mock object for this delegate protocol
  • Set the delegate
  • Set expectations and rejections on what methods should be called. Example
  • Verify

@zhubofei
Copy link
Author

zhubofei commented Jul 17, 2018 •
edited
Loading

@rnystrom gtg?

facebook-github-bot
facebook-github-bot reviewed Jul 25, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@barakwei barakwei mentioned this pull request Aug 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@rnystrom rnystrom rnystrom requested changes

Assignees
No one assigned
Labels
bug CLA Signed
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@zhubofei @jessesquires @rnystrom @facebook-github-bot @bofeizhu