-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Refine dequeueReusableCellOfClass methods for Swift #1388
Conversation
6d4e985
to
9f10ece
Compare
Okay, the build is now failing because of the Xcode build settings. |
Could you update them in this PR so that the build passes? Thanks! |
I believe I now made the minimal changes required to make the tests pass. But running the tvOS tests from Xcode doesn't work without removing certain tests files from the tvOS test target. This doesn't show on CI because the tvOS tests are not run there. Speaking of CI, in terms of speed I think it would be better to move to CircleCI or GitHub Actions; their macOS runners spin up instantly, where Travis can take over 30 minutes. Would you accept a PR that configures CI for a different provider? |
Generated by 🚫 Danger |
@koenpunt I think we'd wanna discuss a change in CI a bit internally before making that decision, but if you'd like to open up an issue and make the case for it there, that'd be great! It'd definitely be helpful for me since I don't have much experience with CIs outside of our own internal one. |
4a9d71f
to
fa2c917
Compare
Hi Koen, thanks for working and fixing the test here! We are working on making a 4.0 release this week, will ping you on this PR once the new release is out, thx! |
45a4dd8
to
3b578a0
Compare
Hi @koenpunt , we released 4.0 yesterday, can you resolve the conflicts on your PR and re-submit? thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for simplifying the APIs for Swift here! It's ready to merge in now, please rebase onto master/stable and test again, thanks!
@@ -1015,7 +1015,7 @@ | |||
GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE; | |||
GCC_WARN_UNUSED_FUNCTION = YES; | |||
GCC_WARN_UNUSED_VARIABLE = YES; | |||
IPHONEOS_DEPLOYMENT_TARGET = 9.3; | |||
IPHONEOS_DEPLOYMENT_TARGET = 9.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -30,7 +30,7 @@ final class EmbeddedSectionController: ListSectionController { | |||
} | |||
|
|||
override func cellForItem(at index: Int) -> UICollectionViewCell { | |||
guard let cell = collectionContext?.dequeueReusableCell(of: CenterLabelCell.self, for: self, at: index) as? CenterLabelCell else { | |||
guard let cell = collectionContext?.dequeueReusableCell(of: CenterLabelCell.self, for: self, at: index) else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat!
of cellClass: T.Type, withReuseIdentifier reuseIdentifier: String, | ||
for sectionController: ListSectionController, at index: Int) -> T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My nit feedback is to use new line for each individual arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you feel strongly about this? I grouped them, since the the cell class and reuse identifier are related, and so are the section controller and the index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont feel too strongly abt it, which I said it's just a 'nit feedback', up to u :))
of cellClass: T.Type, | ||
for sectionController: ListSectionController, at index: Int) -> T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
45a4dd8
to
8150d45
Compare
54d5393
to
3ff3a57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lorixx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Hey @koenpunt, thanks for this new API! Unfortunately we can't take Swift code in the main So, we're going to adjust these new APIs around a bit internally to split out them out into a separate library (probably named We're going to plan to ship this change to Github next week. |
My initial approach was to make it a separate library, but I had a hard time (and didn't spend too much time on it) to make it work with injecting an apinotes file into the framework and then repackaging that as a new library (an approach described here: https://pspdfkit.com/blog/2018/first-class-swift-api-for-objective-c-frameworks/#api-notes). But if there's anything I can do, or if you'd like me to dive into it again, please let me know, I'm happy to help :) |
@natestedman merged this pull request in f1ceedc. |
Summary: We kept getting an error as the following: ``` - ERROR | [OSX] unknown: Encountered an unknown error (The platform of the target `App` (macOS 10.11) is not compatible with `IGListSwiftKit (4.1.0)`, which does not support `macOS`.) during validation. ``` Looks like IGListSwiftKit did not support macos, wondering why this is the case here. Potentially this error was introduced in this PR: Instagram#1388 cc natestedman who might know the context here Differential Revision: D20036436 fbshipit-source-id: 2dbddf0fb029eb165502f1b9d9c0b3be7a964e23
Summary: Pull Request resolved: Instagram#1428 We kept getting an error as the following: ``` - ERROR | [OSX] unknown: Encountered an unknown error (The platform of the target `App` (macOS 10.11) is not compatible with `IGListSwiftKit (4.1.0)`, which does not support `macOS`.) during validation. ``` Looks like IGListSwiftKit did not support macos, wondering why this is the case here. Potentially this error was introduced in this PR: Instagram#1388 cc natestedman who might know the context here Differential Revision: D20036436 fbshipit-source-id: dc77dfae335420fe310ad39a662753af831a1054
Summary: Pull Request resolved: Instagram#1428 We kept getting an error as the following: ``` - ERROR | [OSX] unknown: Encountered an unknown error (The platform of the target `App` (macOS 10.11) is not compatible with `IGListSwiftKit (4.1.0)`, which does not support `macOS`.) during validation. ``` Looks like IGListSwiftKit did not support macos, wondering why this is the case here. Potentially this error was introduced in this PR: Instagram#1388 cc natestedman who might know the context here Reviewed By: natestedman Differential Revision: D20036436 fbshipit-source-id: e9344c847c4be837bc43306a3128ffc87246efd5
Summary: Pull Request resolved: Instagram#1428 We kept getting an error as the following: ``` - ERROR | [OSX] unknown: Encountered an unknown error (The platform of the target `App` (macOS 10.11) is not compatible with `IGListSwiftKit (4.1.0)`, which does not support `macOS`.) during validation. ``` Looks like IGListSwiftKit did not support macos, wondering why this is the case here. Potentially this error was introduced in this PR: Instagram#1388 cc natestedman who might know the context here The solution here is to lint the specs separately in travis, so that for IGListSwiftKit we only lint against ios and tvos. And the other podspecs will still lint against on all platform, including the macos. Reviewed By: natestedman Differential Revision: D20036436 fbshipit-source-id: a2567e439f53593639bff949207af87ff42f6507
Summary: Pull Request resolved: Instagram#1428 We kept getting an error as the following: ``` - ERROR | [OSX] unknown: Encountered an unknown error (The platform of the target `App` (macOS 10.11) is not compatible with `IGListSwiftKit (4.1.0)`, which does not support `macOS`.) during validation. ``` Looks like IGListSwiftKit did not support macos, wondering why this is the case here. Potentially this error was introduced in this PR: Instagram#1388 cc natestedman who might know the context here The solution here is to lint the specs separately in travis, so that for IGListSwiftKit we only lint against ios and tvos. And the other podspecs will still lint against on all platform, including the macos. Reviewed By: natestedman Differential Revision: D20036436 fbshipit-source-id: 67c1578779a20435af0c43428c40a38ad18265b3
Summary: Pull Request resolved: #1428 We kept getting an error as the following: ``` - ERROR | [OSX] unknown: Encountered an unknown error (The platform of the target `App` (macOS 10.11) is not compatible with `IGListSwiftKit (4.1.0)`, which does not support `macOS`.) during validation. ``` Looks like IGListSwiftKit did not support macos, wondering why this is the case here. Potentially this error was introduced in this PR: #1388 cc natestedman who might know the context here The solution here is to lint the specs separately in travis, so that for IGListSwiftKit we only lint against ios and tvos. And the other podspecs will still lint against on all platform, including the macos. Reviewed By: natestedman Differential Revision: D20036436 fbshipit-source-id: 5406cef1825fc4c157fee9ca5856ace81df0508d
Changes in this pull request
Initial suggested implementation for #1387.
Checklist
CHANGELOG.md
for any breaking changes, enhancements, or bug fixes.