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

Make -[IGListAdapter updater] Public, Read-Only #379

Closed
wants to merge 3 commits into from
Closed

Make -[IGListAdapter updater] Public, Read-Only #379

wants to merge 3 commits into from

Conversation

Adlai-Holler
Copy link
Contributor

Hey IGListKit folks! Would you be willing to expose this, so that other objects that are given a list adapter can inspect its updater configuration?

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

@coveralls
Copy link

coveralls commented Dec 30, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 97.978% when pulling 09f36c0 on Adlai-Holler:expose-list-adapter-updater into 0de9632 on Instagram:master.

@jessesquires
Copy link
Contributor

I'm fine with this change. What's your use case?

/cc: @rnystrom ?

@Adlai-Holler
Copy link
Contributor Author

I'd like to be able to assert that a list adapter passed into an API does not use IGListReloadDataUpdater and that the updater is configured with allowsBackgroundReloading=NO. I could modify the API to require the user to keep track of the updater and pass it in alongside the list adapter, but it seems like unnecessary hassle.

@jessesquires
Copy link
Contributor

jessesquires commented Jan 2, 2017 •
edited
Loading

Hmm... but you have to initialize the adapter with an id<IGListUpdatingDelegate>, so couldn't you enforce this then?

@Adlai-Holler
Copy link
Contributor Author

Since the component that receives the adapter is not the one who initializes it, the adapter's owner would have to keep track of the updater separately and provide them both into this API. So @property ASCollectionNode.listAdapter would have to become -[ASCollectionNode setListAdapter:updater:].

rnystrom
rnystrom suggested changes Jan 3, 2017
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.

I've actually thought about exposing this before, so I'm down. There's honestly no drawback.

While we're at it, how about we clean it up a little too?

  • Make sure to remove the internal property
  • Can we rename the property to updater to that it matches the init? Also update the header doc

Sound good?

CHANGELOG.md
@@ -19,6 +19,8 @@ This release closes the [2.1.0 milestone](https://github.com/Instagram/IGListKit

- Added CocoaPods subspec for diffing, `IGListKit/Diffing` and an [installation guide](https://instagram.github.io/IGListKit/installation.html). [Sherlouk](https://github.com/Sherlouk) [(#368)](https://github.com/Instagram/IGListKit/pull/368)

- `IGListAdapter.updatingDelegate` is now public (read-only). [Adlai-Holler](https://github.com/Adlai-Holler) [(#379)](https://github.com/Instagram/IGListKit/pull/379)
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: all of our changelog is in ObjC. Can we change the first part to -[IGListAdapter updatingDelegate]

@facebook-github-bot
Copy link
Contributor

@Adlai-Holler updated the pull request - view changes

@jessesquires jessesquires added this to the 2.1.0 milestone Jan 3, 2017
@jessesquires
Copy link
Contributor

@rnystrom - want to just import this and forward-fix internally?

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

@Adlai-Holler
Copy link
Contributor Author

@rnystrom The internal (readwrite) version of the property unfortunately needs to stay, because the tests use it:

screen shot 2017-01-03 at 3 07 40 pm

@facebook-github-bot
Copy link
Contributor

@Adlai-Holler updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@Adlai-Holler updated the pull request - view changes - changes since last import

@Adlai-Holler
Copy link
Contributor Author

Cool. I've renamed updatingDelegate -> updater and pushed ✨

@Adlai-Holler Adlai-Holler changed the title Make IGListAdapter.updatingDelegate Public, Read-Only Make -[IGListAdapter updater] Public, Read-Only Jan 3, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.978% when pulling 4755f71 on Adlai-Holler:expose-list-adapter-updater into 0de9632 on Instagram:master.

@coveralls
Copy link

coveralls commented Jan 3, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 98.066% when pulling 4755f71 on Adlai-Holler:expose-list-adapter-updater into 0de9632 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.

@coveralls
Copy link

coveralls commented Jan 4, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 97.979% when pulling bf24f58 on Adlai-Holler:expose-list-adapter-updater into 71ce990 on Instagram:master.

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

@rnystrom rnystrom rnystrom approved these changes

Assignees
No one assigned
Labels
CLA Signed enhancement
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Adlai-Holler @coveralls @jessesquires @facebook-github-bot @rnystrom