Skip to content

Replace reusable identifier methods with a C function (#40)#49

Closed
kylehickinson wants to merge 1 commit intoInstagram:masterfrom
kylehickinson:gen-reusable-id
Closed

Replace reusable identifier methods with a C function (#40)#49
kylehickinson wants to merge 1 commit intoInstagram:masterfrom
kylehickinson:gen-reusable-id

Conversation

@kylehickinson
Copy link
Copy Markdown

Changes in this pull request

Replaced the two methods for generating a reusable identifier with an inline C function (as per #40) which has 3 parameters: viewClass (unchanged), nibName (for when #1 is added), and kind. The string is generated following the same pattern as before.

A few things:

  • The current test only tests one of the options. May want to add more tests for each.
  • Not sure if you guys prefer NS_INLINE vs static inline.
  • Not sure if we want to add assertions for empty strings for nibName/kind parameters.

Pull request checklist

Comment thread Source/Internal/IGListAdapterInternal.h Outdated
NS_ASSUME_NONNULL_BEGIN

// Generate the string representation of a reusable view class when registering with a UICollectionView
NS_INLINE NSString *IGListReusableViewIdentifierForClass(Class viewClass, NSString * _Nullable nibName, NSString * _Nullable kind) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

very minor nits: 😄

  • use 3 /// for comment (for xcode docs)
  • docs: "Generates a string..." and . at the end
  • naming: IGListReusableViewIdentifier()

@jessesquires
Copy link
Copy Markdown
Contributor

Looks great! Thanks @kylehickinson ! 💯 🎉

@kylehickinson
Copy link
Copy Markdown
Author

kylehickinson commented Oct 11, 2016

Nits picked! :). -- Except Im an idiot and didn't rename the callee's! Haha wait up

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@kylehickinson updated the pull request - view changes

@jessesquires
Copy link
Copy Markdown
Contributor

The current test only tests one of the options. May want to add more tests for each.

Not sure if we want to add assertions for empty strings for nibName/kind parameters.

It would be great if you could add tests for a supplementary view kind.

And actually, let's remove the nibName param for now 😄

@jessesquires
Copy link
Copy Markdown
Contributor

One thing still up for discussion in #1 is if we want to deal with string names + bundles, or UINib directly. cc @rnystrom

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@kylehickinson updated the pull request - view changes

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@kylehickinson updated the pull request - view changes

@kylehickinson
Copy link
Copy Markdown
Author

Okay, removed nibName, added second (passing) test :)

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@kylehickinson updated the pull request - view changes

@jessesquires
Copy link
Copy Markdown
Contributor

looks good to me! 💯

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@kylehickinson updated the pull request - view changes

@facebook-github-bot
Copy link
Copy Markdown
Contributor

Thanks for importing. If you are a Facebook employee, you can view this diff on Phabricator.

@rnystrom
Copy link
Copy Markdown
Contributor

Amazing, thanks @kylehickinson! Agree we can hold off in the nib stuff for now and add that in once we support it.

@jessesquires jessesquires modified the milestone: 2.0.0 Oct 12, 2016
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.

4 participants