Merge ObjC categories into docs of the extended class #466

esad · 2016-01-29T17:44:57Z

This is work-in-progress on #457.

The idea was to annotate SourceDeclarations representing extensions (both swift and objc) with a link to the parent declaration that is being extended, and then use this as a deduplication key. This kind of unifies the existing approach for swift extensions with new behaviour needed for objc classes/categories.

Furthermore, we found it nice to have category name appear as a mark of the merged methods.

segiddins · 2016-01-30T01:31:14Z

lib/jazzy/sourcekitten.rb

@@ -318,8 +318,8 @@ def self.deduplicate_declarations(declarations)

    # Two declarations get merged if they have the same deduplication key.
    def self.deduplication_key(decl)
-      if decl.type.swift_extensible? || decl.type.swift_extension?
-        [decl.usr]
+      if (decl.type.swift_extension? || decl.type.objc_category?) && !decl.extension_parent.nil?


maybe add a helper method for extension_type?

pcantrell · 2016-02-01T05:06:18Z

Thanks for this, Esad! Two families of questions as I digest the code:

First, what’s the rationale behind computing the extension_parent? It looks like you’re doing it just to replace the deduplication key of [decl.usr] with deduplication_key(decl.extension_parent); is that correct? I’m guessing that’s because Obj-C extensions come back with USRs different from the type they extend…?

If so, I wonder if perhaps the regexp dance you did in your first crack at this was a better plan. It takes a lot of code in this PR to construct extension_parent. Writing an extract_extended_type_usr method might be preferable.

What do those Obj-C USRs look like? Can you provide some examples?

Second, I wonder if it might make sense to unify Swift & Obj-C a bit more, e.g. extension? as a shortcut for swift_extension? || objc_category?, in order to simplify some of those tangled conditionals.

I vaguely remember @jpsim saying that he has a preference for keeping Swift things and Objective-C things somewhat separate, but if he’s amenable to some judicious cross-language generalizing, it could tidy things a bit.

If we are going to keep things separate, then extension_name should be category_name, because extensions are anonymous in Swift.

Nice idea using the category name as a marker, BTW.

esad · 2016-02-01T11:05:15Z

Hi @pcantrell, thanks for your feedback!

Yes, the purpose of going through all extensions to find and set the extension_parent is the deduplication - it struck me as more elegant to simply say "for this extension, use the dedup key of the extended type" than to manually make it match. The extension_name is later used when setting the mark for the merged methods.

The USRs itself in Objective-C look like c:objc(cy)SomeClass@SomeCategory for the category and c:objc(cs)SomeClass for the class itself.

I'll try to refactor this back to a more simple solution, let's see how it looks.

esad · 2016-02-01T12:04:41Z

I've updated the PR with a simpler approach. What I really disliked about my first try was manually twiddling USRs so that they match, now I rely on extracted extended class name instead.

Same method that is is used to extract the extended class name also extracts the category name, which is used for the mark.

(The new approach should also merge fine with #462)

segiddins · 2016-02-03T17:16:40Z

Before merging, this will need a rebase to get rid of the merge commit

segiddins · 2016-02-03T17:17:33Z

lib/jazzy/source_declaration.rb

+    # of the extended objc class and the category name itself, i.e.
+    # ["NSString", "MyMethods"], nil otherwise.
+    def objc_category_name
+      type.objc_category? && name.split(/[\(\)]/) || nil


This could just be name.split(/[\(\)]/) if type.objc_category?

segiddins · 2016-02-03T17:19:29Z

This warrants adding categories to an integration spec, I think

esad · 2016-02-04T15:46:47Z

I addressed the above PR remarks, but I'm blocked by integration specs not passing on the current master. Waiting to resolve this first before I update them.

pcantrell · 2016-02-06T16:43:23Z

I like the way this ended up. Not too complicated; things makes sense. I'm dropping a few minor notes into the code, and after those I'd say it's good to merge.

+1 to Sam’s comment that this warrants an addition to the specs, perhaps just a small addition to misc_jazzy_features — though I think, given this project’s somewhat loose coupling between features and tests, that could be a separate PR if it proves to be a hassle.

pcantrell · 2016-02-06T16:46:52Z

lib/jazzy/source_declaration.rb

+    # of the extended objc class and the category name itself, i.e.
+    # ["NSString", "MyMethods"], nil otherwise.
+    def objc_category_name
+      name.split(/[\(\)]/) if type.objc_category?


The name objc_category_name implies that this returns a string, not an array — and that seems like the right behavior.

Do you think I should split this method into two: one to return the category name and the other to return the extended class name? Or rename the above one to match the method signature?

Now that I understand better how you’re using it, I'd suggest renaming it to make it clear that it returns two values.

esad · 2016-02-09T17:39:18Z

@segiddins What would be the best way forward to add integration tests for the above? I tried adding some objc categories to the MiscJazzyFeatures project, but the SourceKitten ignores those (even though I added them to the umbrella header).

It seems that jazzy currently doesn't support mixed Swift/ObjC projects? Should I add a new MiscJazzyObjcFeatures project to the integration specs?

segiddins · 2016-02-09T17:43:34Z

Yeah, I think a new MiscJazzyObjcFeatures project would probably be the best bet

esad · 2016-02-11T21:56:36Z

I've added integration specs for this feature. As pointed out in the child PR, I'm bit unsure how this workflow with 2 dependent separate repos should work. Can you merge the child PR first, so that I can point to it here and we can have CI pass on this PR?

esad · 2016-02-11T21:57:56Z

After integration specs pass I'll address the remaining code style issues.

pcantrell · 2016-02-11T22:56:25Z

The integration specs process is a bit of a mystery to me too at times, but I merged your PR and hopefully that unblocks this merge.

As you do the last little tidying work, please feel free to edit .rubocop.yml to make it so you can do name, _ = instead of name, =. Anybody else feel free to pipe up in disagreement, but I find the latter terrible.

Thanks for bearing with this whole process!

…tend

esad · 2016-02-12T10:48:53Z

@pcantrell @segiddins I've addressed all issues we discussed above, added integration specs and rebased the PR. Let me know if you feel there's something else missing.

esad · 2016-02-24T19:58:12Z

I just wanted to let you know that we've rebuilt PSPDFKit docs with this patch applied and it worked fine, with categories getting merged nicely into the class docs.

pcantrell · 2016-02-24T20:32:18Z

I'd love to get this merged! It's only blocked on the CI failing. It looks like the integration specs are building Moya with a slightly different version of AF that the expected output recorded.

I have to speed-prep a talk I'm doing tonight, but @esad (or anyone else), if you have a chance to update the expected output, I can approve the merges.

esad · 2016-03-01T09:38:21Z

@pcantrell As soon as https://github.com/realm/jazzy-integration-specs/pull/11/commits in jazzy-integration-specs gets merged I will update this PR

esad · 2016-03-01T19:13:24Z

@pcantrell Looks like the specs are passing again!

pcantrell · 2016-03-01T20:58:20Z

Merging! Hooray!

Thanks for all the hard work and the patience.

Merge ObjC categories into docs of the extended class

jpsim · 2016-03-18T00:14:12Z

This is great! Thanks @esad for your hard work, and @pcantrell & @segiddins for the reviews! 👍

@esad

thanks @esad!

esad mentioned this pull request Jan 29, 2016

Merge Obj-C categories into interface #457

Closed

segiddins reviewed Jan 30, 2016
View reviewed changes

esad force-pushed the master branch from 30f2542 to 429ba58 Compare February 2, 2016 13:09

segiddins reviewed Feb 3, 2016
View reviewed changes

esad mentioned this pull request Feb 4, 2016

Order of classes not stable #473

Closed

pcantrell reviewed Feb 6, 2016
View reviewed changes

esad mentioned this pull request Feb 11, 2016

Add new "misc_jazzy_objc_features" integration spec realm/jazzy-integration-specs#10

Merged

Merge docs from Objective-C categories into docs of the class they ex…

da8cb39

…tend

esad force-pushed the master branch from 923f5df to 7922036 Compare February 12, 2016 10:47

esad mentioned this pull request Mar 1, 2016

Update moya execution output realm/jazzy-integration-specs#11

Merged

Add MiscJazzyObjCFeatures integration specs

9474cc8

esad force-pushed the master branch from 7922036 to 9474cc8 Compare March 1, 2016 18:41

pcantrell added a commit that referenced this pull request Mar 1, 2016

Merge pull request #466 from PSPDFKit-labs/master

7d7c120

Merge ObjC categories into docs of the extended class

pcantrell merged commit 7d7c120 into realm:master Mar 1, 2016

esad mentioned this pull request Mar 30, 2016

Bump version to 0.6.0 #501

Closed

jpsim added a commit that referenced this pull request Mar 30, 2016

add changelog entry for #466

a485292

thanks @esad!

jpsim mentioned this pull request Apr 6, 2016

ObjC Misc features integration test #376

Closed

pigeondotdev modified the milestone: The Past Nov 22, 2016

pigeondotdev added github-review-accepted enhancement labels Nov 22, 2016

pigeondotdev assigned jpsim, segiddins and pcantrell Nov 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge ObjC categories into docs of the extended class #466

Merge ObjC categories into docs of the extended class #466

esad commented Jan 29, 2016

segiddins Jan 30, 2016

pcantrell commented Feb 1, 2016

esad commented Feb 1, 2016

esad commented Feb 1, 2016

segiddins commented Feb 3, 2016

segiddins Feb 3, 2016

segiddins commented Feb 3, 2016

esad commented Feb 4, 2016

pcantrell commented Feb 6, 2016

pcantrell Feb 6, 2016

esad Feb 9, 2016

pcantrell Feb 9, 2016

esad commented Feb 9, 2016

segiddins commented Feb 9, 2016

esad commented Feb 11, 2016

esad commented Feb 11, 2016

pcantrell commented Feb 11, 2016

esad commented Feb 12, 2016

esad commented Feb 24, 2016

pcantrell commented Feb 24, 2016

esad commented Mar 1, 2016

esad commented Mar 1, 2016

pcantrell commented Mar 1, 2016

jpsim commented Mar 18, 2016

Merge ObjC categories into docs of the extended class #466

Merge ObjC categories into docs of the extended class #466

Conversation

esad commented Jan 29, 2016

segiddins Jan 30, 2016

Choose a reason for hiding this comment

pcantrell commented Feb 1, 2016

esad commented Feb 1, 2016

esad commented Feb 1, 2016

segiddins commented Feb 3, 2016

segiddins Feb 3, 2016

Choose a reason for hiding this comment

segiddins commented Feb 3, 2016

esad commented Feb 4, 2016

pcantrell commented Feb 6, 2016

pcantrell Feb 6, 2016

Choose a reason for hiding this comment

esad Feb 9, 2016

Choose a reason for hiding this comment

pcantrell Feb 9, 2016

Choose a reason for hiding this comment

esad commented Feb 9, 2016

segiddins commented Feb 9, 2016

esad commented Feb 11, 2016

esad commented Feb 11, 2016

pcantrell commented Feb 11, 2016

esad commented Feb 12, 2016

esad commented Feb 24, 2016

pcantrell commented Feb 24, 2016

esad commented Mar 1, 2016

esad commented Mar 1, 2016

pcantrell commented Mar 1, 2016

jpsim commented Mar 18, 2016