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.

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

Autolink declarations #490

Closed
wants to merge 30 commits into from
Closed

Conversation

esad
Copy link
Collaborator

@esad esad commented Feb 24, 2016

This solves #483 - autolinking return and argument types in the method declarations in both Swift and Objective-C.

I've reused the same autolinking logic already used for text, with an additional flag that instead of running it inside <code>, runs it inside rogue (syntax highlighting) emitted <span> tags of special classes.

The diff for the integration specs will be quite huge as this affects lots of generated html - so it would be a good idea if you someone can take a look at what this does to their own docs - the output for both Realm ObjC and Swift looks good (and has nice links in the return and argument types!)

@@ -448,6 +458,7 @@ def self.autolink(docs, root_decls)
doc.abstract = autolink_text(doc.abstract, doc, root_decls)
doc.return = autolink_text(doc.return, doc, root_decls) if doc.return
doc.children = autolink(doc.children, root_decls)
doc.declaration &&= autolink_text(doc.declaration, doc, root_decls, true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok perhaps this could be rewritten to be consisent with the above doc.return = autolink(..) if doc.return style?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I refactor this to doc.declaration = .. if doc.declaration the line gets too long and rubocop complains :( Should I refactor anyway and disable the rubocop warning?

@jpsim
Copy link
Collaborator

jpsim commented Mar 18, 2016

Woohoo! This is nice. Could you please address @segiddins's style feedback, push the changes to the integration specs (you should have commit access now 🎉) and then we'll be able to take a look at the specs diff and look at the changes in a web browser.

Thanks!

@jpsim
Copy link
Collaborator

jpsim commented Apr 6, 2016

@esad now that I've made a few fixes to jazzy and generally brought it back to good health (or at least I hope), could you please revive this so we can include it in the next release?

@esad esad force-pushed the autolink_decls branch from 989be31 to 63bca2f Compare April 6, 2016 09:32
@esad
Copy link
Collaborator Author

esad commented Apr 6, 2016

@jpsim Almost there. Rubocop is complaining about a few things, so I wanted to ask for advice how to best deal with it:

lib/jazzy/sourcekitten.rb:440:71: C: Method has too many lines. [24/20]
lib/jazzy/sourcekitten.rb:440:71: C: Perceived complexity for autolink_text is too high. [8/7]
lib/jazzy/sourcekitten.rb:480:70: C: Line is too long. [98/80]
        doc.declaration = autolink_text(doc.declaration, doc, root_decls, true) if doc.declaration

Not sure if I can split the autolink_text method meaningfully to avoid complexity and too many lines errors. Can I just disable Rubocop here?

Regarding the line length offence for doc.declaration - should we switch all three invocations to use &&= syntax?

…uped and ungrouped docs. Necessary for autolink to be able to correctly find enums.
@jpsim
Copy link
Collaborator

jpsim commented Apr 6, 2016

This is a great start @esad! There are a few behaviors that I think should be addressed before getting this in:

First, disabling auto-linking to the same page you're already on:

image

This is a screenshot taken from building the integration specs with this branch, and includes links from Alamofire's Manager to Manager.

Second, disabling links to types not defined in the module:

image

This is another screenshot taken from building the integration specs with this branch, and includes links from Alamofire's SessionDelegate to NSURLRequest because Alamofire contains extensions to NSURLRequest.

Those are the only two issues I can identify with this for now. Another modification we could consider is preserving the syntax color of the type from rouge, distinguishing between hyperlinks and non-links some other way, like underlines... I'm not too picky if we do this or not.

@jpsim
Copy link
Collaborator

jpsim commented Apr 6, 2016

As for the rubocop violations, do whatever you prefer there.

@DangerCI
Copy link

DangerCI commented Jun 7, 2016

      <tr>
    <td>:white_check_mark:</td>
    <td data-sticky="true"><del><p>Please include a CHANGELOG entry. 

You can find it at CHANGELOG.md.






















        1 Error
    
  </th>
 </tr>
🚫

Please include a CHANGELOG entry. You can find it at CHANGELOG.md.

Please include a CHANGELOG entry.
You can find it at CHANGELOG.md.

Please include a CHANGELOG entry.
You can find it at CHANGELOG.md.

Please include a CHANGELOG entry.
You can find it at CHANGELOG.md.

Please include a CHANGELOG entry.
You can find it at CHANGELOG.md.

Please include a CHANGELOG entry.
You can find it at CHANGELOG.md.

      <tr>
    <td>:white_check_mark:</td>
    <td data-sticky="true"><del>This PR may need tests.</del></td>
  </tr>
        :white_check_mark: Yay.
    
  </th>
 </tr>
        1 Message
    
  </th>
 </tr>
📖 Note, we hard-wrap at 80 chars and use 2 spaces after the last line.

Generated by 🚫 danger

@esad
Copy link
Collaborator Author

esad commented Jun 8, 2016

@jpsim

I've addressed the two things you mentioned above, updated the integration specs and tweaked rubocop rules a bit. Travis is failing because of missing CHANGELOG entry, I was think that you may want to craft that one?

The fact that we don't autolink on the same page anymore will also affect previous autolinking logic, where this was not respected, so for example in the abstract for https://realm.io/docs/objc/latest/api/Classes/RLMRealm.html#/c:objc(cs)RLMRealm(cm)defaultRealm, the RLMRealm won't be autolinked.

I've also updated the logic not to perform autolinking to extensions (such as NSURLRequest).

I had some issues with integration specs for MiscJazzyFeatures having extra *.md files residing in a Docs/ subdirectory where also the autogenerated docs go per default, so I moved those to Extra/*.md.

As for Rubocop, I've tried to disable as little checks as possible and comply with the style, but I had to disable complexity checks for the autolinking method, however I think it's still quite readable. I've also globally disabled a recently introduced style check for multiline block chaining that IMHO makes no sense (see rubocop/rubocop#2338), which was also causing some old code in sourcekitten.rb to fail the check.

I've also updated the code to autolink declarations from the the "other language declaration", introduced in v0.6.1.

@jpsim
Copy link
Collaborator

jpsim commented Jun 8, 2016

Hey @esad, there were conflicts since merging #569 so I worked to resolve those and rebuild the integration specs to better review this.

I did this in the jazzy@jp-autolink-decls and specs@jp-autolink-decls branches.

The main issue I can find with this are that some declarations now link to themselves:

image

@esad
Copy link
Collaborator Author

esad commented Jun 9, 2016

@jpsim I've merged your changes and extended the check to prevent autolinking to the same page in the cases like the one you mentioned above. Can you have another look?

@jpsim
Copy link
Collaborator

jpsim commented Jun 9, 2016

I'll review this again today @esad. I have to thank you for your patience, I know it's been tedious to keep this updated 😬

@segiddins
Copy link
Collaborator

CI is only failing due to lack of a CHANGELOG entry

@jpsim
Copy link
Collaborator

jpsim commented Jun 9, 2016

Previously, we'd link to other child declarations on the current page, like properties on class documentation. These no longer autolink. See the startRequestsImmediately reference in Alamofire's docs:

before:

image

after:

image

@esad
Copy link
Collaborator Author

esad commented Jun 9, 2016

@jpsim I thought that's what the intended behavior per your comment in #490 (comment) was - can you clarify in more detail what do you think the behavior should be when it comes to linking to the same page? Link everything except to itself and to the toplevel declaration of the current page (Manager in your example?)

@jpsim
Copy link
Collaborator

jpsim commented Jun 9, 2016

Sorry for not being clear, yes I agree with your description of the intended behavior:

Link everything except to itself and to the top-level declaration of the current page

esad added 2 commits June 10, 2016 13:02
@esad
Copy link
Collaborator Author

esad commented Jun 10, 2016

@jpsim Updated. Could you have one more look?

@jpsim
Copy link
Collaborator

jpsim commented Jun 10, 2016

Looks good, I've continued this in #588

@esad
Copy link
Collaborator Author

esad commented Jun 10, 2016

@jpsim Thanks for taking over with the latest merge, looks good.

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.

5 participants