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

Display doc comments for all swift extensions #819

Merged
merged 4 commits into from
May 18, 2017
Merged

Display doc comments for all swift extensions #819

merged 4 commits into from
May 18, 2017

Conversation

johnfairh
Copy link
Collaborator

@johnfairh johnfairh commented May 16, 2017

See #454, #620.
Now sourcekitten is passing on doc comments of extensions, this PR displays them. The corner case where this wasn't working is Swift extensions of Objective C classes like Foundation.NSDate: in this case sourcekit does not provide the full_as_xml field.

Specs changes show one fix in Siesta demonstrating exactly this, and another 'fix' in realm-swift where a previously missed doc comment is now displayed --- the declaration shouldn't be present at all, I think, but that's another story + not a regression here...

edit with latest push: also propose dropping the 'undocumented' label on swift extensions of objc classes without doc comments, for consistency with other extensions + because they do not contribute to undocumented%. Spec changes reflect that.

johnfairh added 2 commits May 16, 2017 18:26
Swift extensions of Foundation classes like `NSString` are
given their doc comments rather than 'Undocumented'.
@SDGGiesbrecht
Copy link
Contributor

What happens when no documentation is provided?

Will this introduce new entries in undocumented.json, or will it just remove the peculiar “Undocumented” note from the generated documentation?

It seems odd to me to require documentation when the symbol is already documented at its declaration (even if in another module), and the extension documentation is inaccessible to Quick Help and friends.

On the other hand, thumbs up if this stops Jazzy from publishing the nonsense text “Undocumented” under each extended type. That has been annoying me for a while.

I don’t mind if I will have to add :nodoc: or something similar to extensions to suppress warnings, but I’m concerned that it be possible to do in a way that does not make the symbol itself vanish from the documentation—especially if its descendants disappear with it.

@johnfairh
Copy link
Collaborator Author

If no doc were provided then you would get the 'nonsense' text 😄 as today, no change.

The only thing this change does is display the doc comment -- if you wrote one -- instead of 'Undocumented'. I think it is often natural + useful to write doc comments on extensions of existing types to explain their purpose in the context of a module's complete documentation; several user issues along this line.

So, you would be able to get rid of 'undocumented' with an empty doc comment '///'.

Extensions of types from other modules never contribute to undocumented.json.

...so it is a bit unhelpful to stick 'undocumented' on them without warning.
And some extensions have no need of documentation because covered in content docs / would just be 'Some useful methods on Foo'. (plus your point wrt Xcode).
And extensions of Swift types (local or foreign module) do not behave like this.

OK, I'm convinced: we should never see this particular 'Undocumented'.

New proposal for undocumented symbols:

  1. Display 'Undocumented.' for the abstract if and only if the symbol goes in undocumented.json and so affects documented%: we want the user to write some docs.
  2. Otherwise display the doc comment as the abstract, which may be blank: we don't mind if the user doesn't write docs, but if they do then we'll display them.

Thanks a lot for the comments.

@SDGGiesbrecht
Copy link
Contributor

Sounds perfect, @johnfairh. Thanks for listening.

@jpsim jpsim merged commit 3e45911 into master May 18, 2017
@jpsim jpsim deleted the issue-454 branch May 18, 2017 23:15
@jpsim
Copy link
Collaborator

jpsim commented May 18, 2017

Awesome, thanks @johnfairh 👏

@johnfairh
Copy link
Collaborator Author

cheers!

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.

3 participants