Skip to content

Swap NSUInteger to NSInteger Public API#205

Closed
surayashivji wants to merge 3 commits intoInstagram:masterfrom
surayashivji:nsintegerswap
Closed

Swap NSUInteger to NSInteger Public API#205
surayashivji wants to merge 3 commits intoInstagram:masterfrom
surayashivji:nsintegerswap

Conversation

@surayashivji
Copy link
Copy Markdown
Contributor

@surayashivji surayashivji commented Nov 16, 2016 •
edited
Loading

Changes in this pull request

Swapped NSUInteger to NSInteger in public headers. Fixed a test in IGListSectionMapTests.m to pass with NSInteger. For issue #200!

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

@surayashivji surayashivji changed the title Nsintegerswap Swap NSUInteger to NSInteger Public API Nov 16, 2016
@rnystrom
Copy link
Copy Markdown
Contributor

rnystrom commented Nov 16, 2016

Looks like a race condition w/ #206 😂

I can take this one since it was first in, but we should probably revert all the docs/ and podfile changes. We're tracking to update that in #181 (right before 2.0.0 cut).

@rnystrom rnystrom mentioned this pull request Nov 16, 2016
Closed
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Nov 16, 2016

@surayashivji updated the pull request - view changes

@surayashivji
Copy link
Copy Markdown
Contributor Author

surayashivji commented Nov 16, 2016

Cleaned up the branch!

@rnystrom
Copy link
Copy Markdown
Contributor

rnystrom commented Nov 16, 2016

Gonna let CI ✅ then we're g2g!

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 16, 2016

Coverage Status

Coverage remained the same at 91.441% when pulling a9fe9e5 on surayashivji:nsintegerswap into 204f7d5 on Instagram:master.

@jessesquires
Copy link
Copy Markdown
Contributor

jessesquires commented Nov 16, 2016

LGTM. thanks @surayashivji ! 🎉

@jessesquires jessesquires added this to the 2.0.0 milestone Nov 16, 2016
jessesquires
jessesquires suggested changes Nov 16, 2016
View reviewed changes
Copy link
Copy Markdown
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.

Oops, forgot one thing.

We need to add this to the CHANGELOG.md.

This is technically a breaking change.

@rnystrom -- Swift APIs will change from UInt to Int, correct?

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 16, 2016

Coverage Status

Coverage remained the same at 91.441% when pulling a9fe9e5 on surayashivji:nsintegerswap into 204f7d5 on Instagram:master.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Nov 16, 2016

@surayashivji updated the pull request - view changes

@surayashivji
Copy link
Copy Markdown
Contributor Author

surayashivji commented Nov 16, 2016 •
edited
Loading

Updated CHANGELOG.md 👍

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 16, 2016

Coverage Status

Coverage remained the same at 91.441% when pulling 69facbd on surayashivji:nsintegerswap into 204f7d5 on Instagram:master.

@rnystrom
Copy link
Copy Markdown
Contributor

rnystrom commented Nov 16, 2016

@surayashivji last thing w/ the changelog, can you use the same format as items in Enhancements? Name (PR#). I believe the commit hash will change due to how we sync PRs.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Nov 16, 2016

@surayashivji updated the pull request - view changes

@surayashivji
Copy link
Copy Markdown
Contributor Author

surayashivji commented Nov 16, 2016 •
edited
Loading

Updated CHANGELOG.md-- thank you for your patience 😂

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Nov 16, 2016

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

@jessesquires
Copy link
Copy Markdown
Contributor

jessesquires commented Nov 16, 2016

Thanks @surayashivji ! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jessesquires jessesquires jessesquires requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@surayashivji @rnystrom @facebook-github-bot @coveralls @jessesquires