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

Fixes to autolinking #856

Merged
merged 4 commits into from
Sep 21, 2017
Merged

Fixes to autolinking #856

merged 4 commits into from
Sep 21, 2017

Conversation

johnfairh
Copy link
Collaborator

Autolink to symbols from parameter documentation (#789) and from external markdown files, readme / documentation / custom abstract (#715).
Autolink correctly to symbols containing & < > (#805).

Fix something I broke earlier that caused undocumented parameters to appear in the parameter tables -- found while autolinking parameters.

Specs changes:

  • RealmObjC - add linking from parameter descriptions
  • Moya - delete empty parameters
  • Alamofire - add linking from parameters and README.md
  • RealmSwift - delete empty parameters; add linking from parameters
  • Sierra - add linking from parameters
  • MiscJazzyFeatures - add new tests

@jpsim
Copy link
Collaborator

jpsim commented Jul 17, 2017

Thanks for doing this!

Reviewing the changes to the specs, there's a somewhat-regression in Alamofire: Notification.Name.Task is no longer listed in the nav sidebar

Reviewing this also highlighting a pre-existing issue: how should we deal with nested types?

For example, in Alamofire, DownloadRequest is a class, but DownloadRequest.DownloadOptions is a struct, and due to the sidebar's nesting, they're both under the "Classes" heading.

image

This is especially odd with Extensions, since Notification.Name and Notification.Key are really declarations, not extensions, even though they're declared in extensions on Notification.

@johnfairh
Copy link
Collaborator Author

On the Notification.Name.Task thing: this happened during #847 + #848, the extensions-of-nested-types PRs.

Before these, we were confused and the sidebar said Notification/Task which is wrong.
With the sourcekitten part, 847, the sidebar said “Notification.Name”/Task (but also “Realm.Configuration” elsewhere).
With the Jazzy part, 848, the sidebar wants to say Notification/Name/Task but can’t because it’s limited to two levels.

Maybe it would be better to avoid the unflattening for extensions of external types so it would read:

	Notification
		Key
	Notification.Name
		Task

...that way the top-level item is always a type that is being extended, even if that type is nested, and any items under it have been declared by the project. The current version mixes up stuff in the extension (Key) with stuff already in the type that is also being extended (Name).

Well, I’ve convinced myself...

@johnfairh
Copy link
Collaborator Author

Some inconclusive thoughts on nested types in general.
tl;dr: smush everything together or special-case types from extensions?

From a reader’s point of view I prefer the nested types under their parents: authors create nested types because they are strongly related to the parent type -- I'd like to read about them all together. AF’s DownloadRequest is a good example there.

Agree it’s a bit odd to find a nested struct in the Classes category. We could simplify these default categories into New Types/Extended Types/Functions/Global vars (or some smaller/bigger set,) merging (say) classes/structs/protocols/typealiases/enums.

As a reader, when coming in to read a project’s docs and find things from the left nav, I don’t think I initially care if a type is a struct or something else -- this is a detail of how I can use the type. I don’t have a use case for the default layout (“today I want to learn about all structs this module provides”?). Even as a project author I’m not sure it helps me.

The merged category would be larger, for sure. Maybe keeping the category lengths down is better UI. You probably went through this at the start of Jazzy! I don’t know that anyone will be seriously confused by finding a nested struct in the classes category.


I can see how the extensions category could seem worse that the others. I've been thinking of it as 'extended types [and their children]' category and so it has felt natural for nested types to show up there.

It’s easy enough to imagine moving docs for a new struct from an extension to the Structs category. This would work nicely with Alamofire because it uses the extension solely for a namespace. But if the project declared a type in an extension along with members for working on it/relating it to the extended type, then we'd end up with the pure extension containing just members and the new nested type elsewhere in the docs tree. Maybe this is good - gives a bit more prominence to the new type.

@johnfairh johnfairh force-pushed the jf-autolink-fixes branch from c69f90b to 0f0afe4 Compare July 30, 2017 13:37
Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

👍 sorry for the very long delay.

@johnfairh johnfairh merged commit d5a6cfd into master Sep 21, 2017
@johnfairh
Copy link
Collaborator Author

Thanks! Merged.

@johnfairh johnfairh deleted the jf-autolink-fixes branch September 21, 2017 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants