Swift name annotations #593
Conversation
Generated by 🚫 Danger |
|
@robertjpayne Nice! 👍 🥇 Glanced through this and looks pretty good. cc @rnystrom I'm down for this, but it will be a huge breaking change for 3.0 (but that's probably OK) |
|
@jessesquires the only think that is actually kinda frustrating is I sort of feel like EDIT: actually it's probably better the other way around, to do |
There was a problem hiding this comment.
@jessesquires @robertjpayne What do you think about updating the guides that use Swift as well? Do that in a follow up or now?
I'm so down w/ this though!
CHANGELOG.md
Outdated
| @@ -9,6 +9,23 @@ This release closes the [3.0.0 milestone](https://github.com/Instagram/IGListKit | |||
|
|
|||
| ### Breaking Changes | |||
|
|
|||
| - Added Swift annotation names removing IGL prefixes from API's and improving those of C functions. [Robert Payne](https://github.com/robertjpayne) | |||
There was a problem hiding this comment.
nit: can you link to the PR? See other changelog entries as an example
|
I think it's probably wise to update the guides at the same time -- I may need a few days to get there though busy end of the week. |
|
@robertjpayne - No worries! |
|
@jessesquires @rnystrom So the documentation is auto generated, I'm not super familiar with Jazzy so I'm not going to touch that, I would hope it can auto-generate the Swift API's vs the ObjC apis? As far as the guides go, do we assume they are all written for Swift users? I'm curious about what should happen to documentation lines like:
|
|
@robertjpayne Hm, that's a good point. We should leave the prefixes for everything, except example docs, etc. that use Swift. So that means we should leave header doc comments alone. Actually... jazzy might handle this correctly since it will show ObjC and Swift APIs. I won't know for sure until this is merged and I can run the docs scripts to see. Maybe let's hold off on this for now -- @rnystrom and I can take care of refining docs for the release. |
|
@jessesquires sounds good -- lemme know if I can help with anything else! |
|
@robertjpayne ack, mind rebasing? Should just be replacing |
|
Sorry for those merge conflicts 😊 |
|
@robertjpayne updated the pull request - view changes |
|
@robertjpayne updated the pull request - view changes |
|
Alright @jessesquires @rnystrom should be up to date with master now ( ran all the tests and built all the examples ) -- apologies the merge was rather manual instead of a clean rebase because I didn't really feel like using diff merge on every example file! Also updated the changeling with a link to this PR! |
|
Can we get Travis to re-test? I think the timeouts are invalid -- locally all the tests are passing… |
|
👍 |
|
@robertjpayne updated the pull request - view changes |
|
@robertjpayne updated the pull request - view changes |
|
@jessesquires should we still try to do this for 3.0? Huge rebase necessary. Prepping for documentation updates if we're going to do it. Tbh I could go either way. I like the idea, but it does make communication a little trickier (ObjC vs Swift language). I'm still on board tho. |
|
@rnystrom - yeah I still think this is a good change for Swift clients. The foundation overlays from Apple do this for certain classes. (NSData --> Data, NSUserDefaults --> UserDefaults, etc.) As long as all that's happening is removing the |
|
There was a few API changes on the C methods (just added argument labels) but Xcode adds "FIXME"'s for any of those it breaks. I could have left the argument labels off but I felt it actually helped explain the parameters |
|
Thanks @robertjpayne ! 💯 do you mind rebasing and fixing conflicts, then we'll get this imported and merged? |
|
@jessesquires when is it best to land? I can rebase it again (it's actually easier to just use the regex I have to redo it than rebase it). Are you planning on merging some day specific this week so I can line it up to avoid another rebase? |
|
@robertjpayne we can land at any time (we manually click a button), so its on me not merging this fast enough. If you want to, rebase & push your changes then tweet/DM me and I'll init a land. Sorry for the constant rebasing 😞 Also we don't have anything to land today, so it's a good day for us to get this in |
|
@robertjpayne updated the pull request - view changes |
|
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Changes in this pull request
This adds
NS_SWIFT_NAMEannotations to all public API's to provide cleaner integration into Swift:IGListDiff([], [], .equality)=>ListDiff(oldArray: [], newArray: [], option: .equality)While this is a large API change it should be as easy as:
(IGList)([^K])toList$2in Xcode with a scope set to SwiftI have not updated the documentation to reflect this yet, I am totally willing to do so but before I sink that amount of time into it I wanted to see if the Instagram team is even open to this change!
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.