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

automatically hyperlink references to top-level declarations #324

Merged
merged 3 commits into from
Oct 29, 2015

Conversation

jpsim
Copy link
Collaborator

@jpsim jpsim commented Oct 24, 2015

This dead simple implementation goes a pretty long way, but there are a few cases that could be improved:

  1. References in code samples are automatically hyperlinked, which doesn't always look very nice. I'm not sure what the best way to prevent this would be.
  2. To make autolinking work with declarations deeper than top-level ones, we'd have to only autolink if it's on the same page (IMO).

Nevertheless, I don' think these limitations are enough to prevent adding this functionality. /cc @segiddins @orta.

@orta
Copy link
Collaborator

orta commented Oct 24, 2015

Yeah, shippit.

def self.names_and_urls(docs)
names_and_urls = docs.flat_map do |doc|
# FIXME: autolink more than just top-level items.
[{:name => doc.name, :url => doc.url}] # + names_and_urls(doc.children)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why flat_mapping to a single-item array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See the comment

@segiddins
Copy link
Collaborator

We might want to apply a more toned-down style to auto links as compared to normal links?

@segiddins
Copy link
Collaborator

What's the performance hit on this? I can imagine it could get to be quite large...

@segiddins
Copy link
Collaborator

References in code samples are automatically hyperlinked, which doesn't always look very nice. I'm not sure what the best way to prevent this would be.

Maybe by doing the auto linking at an earlier stage, where we can only apply it to the sections of text we want to?

@jpsim
Copy link
Collaborator Author

jpsim commented Oct 24, 2015

What's the performance hit on this? I can imagine it could get to be quite large...

In practice, building the Xcode project takes the vast majority of the running time, so optimizing this is unlikely to have a large impact on overall run time for most projects. That being said, if you see obvious perf improvements that could be made, I'm all ears.

Maybe by doing the auto linking at an earlier stage, where we can only apply it to the sections of text we want to?

This wouldn't help. Consider the following scenario:

public class MyClass {}

/**
 This is my function.

     This is a code sample that references MyClass.
 */
public func myFunc() {}

We pass the whole comment to be parsed as Markdown. We can't reliably know where the code blocks are until the Markdown is parsed, and then afterwards we could avoid autolinking references enclosed within <code> or <pre> tags, but how can you avoid greedy matches?

@jpsim
Copy link
Collaborator Author

jpsim commented Oct 24, 2015

References in code samples are automatically hyperlinked, which doesn't always look very nice. I'm not sure what the best way to prevent this would be.

The best way I can think of to do this would be to strip all URLs out of code blocks as part of jazzy_markdown.

end

def self.autolink(docs, data)
docs.map do |doc|
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be each

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so. The intent is to return an array of transformed elements, not to mutate the elements in-place. So if this is what map is doing (passing mutable references to the elements), then making a copy of the doc block argument would be more "pure", but functionally the same I believe.

@jpsim
Copy link
Collaborator Author

jpsim commented Oct 24, 2015

We might want to apply a more toned-down style to auto links as compared to normal links?

Good idea. What do you think of a new CSS style that only applies the blue link color on hover?

jpsim added a commit that referenced this pull request Oct 29, 2015
automatically hyperlink references to top-level declarations
@jpsim jpsim merged commit e3c90fa into master Oct 29, 2015
@jpsim jpsim deleted the jp-autolink branch October 29, 2015 00:48
@pigeondotdev pigeondotdev modified the milestone: The Past Nov 22, 2016
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.

4 participants