Support documenting tuple return value elements individually#1473
Support documenting tuple return value elements individually#1473amanthatdoescares wants to merge 4 commits into
Conversation
d-ronnqvist
left a comment
There was a problem hiding this comment.
Some initial high-level feedback:
- You are modifying quite a lot of code but only testing that the list item extractor extracts the values.
- You are adding or modifying a fair amount of
publicAPI. Because public API can't (easily) be changed after it's been added; all of those changes will require a deeper discussion and require a higher level of scrutiny. - There are some oddities in the design where return values use properties or types named "parameters" or use API that's intended for parameters.
|
The "Testing" section of the pull request template is intended for instructions for how a human can verify the changes end-to-end. This is in addition to the expectation that all changes are covered by automated tests. |
|
Thanks for the review. I’ll address the three points:
I’ll push updates once these are done. |
8a36b97 to
43c9115
Compare
|
I kept the implementation minimal based on the earlier feedback: no new public API or render section, and the “same as Parameters” behavior is handled during rendering. To keep the PR small, I left out tuple-count validation and extra automated tests (validator/renderer). I’m happy to add those in a follow-up if needed. |
There was a problem hiding this comment.
None of the changes in this file are covered by tests.
There was a problem hiding this comment.
I have added a test for this now, please check and review
| let outline = listItem.extractInnerTagOutline() | ||
| if !outline.isEmpty, | ||
| let innerList = listItem.children.compactMap({ $0 as? UnorderedList }).first { | ||
| returns.append(Return(contents: [innerList], range: extractedTag.range)) |
There was a problem hiding this comment.
Can you explain to me what this is meant to accomplish?
There was a problem hiding this comment.
That change is a bit of a hack.
What it’s trying to do is: when - Returns: is written in the outline form
/// - Returns:
/// - quotient: ...
/// - remainder: ...
It keeps the inner UnorderedList intact inside a single Return, so the HTML renderer can recognize that shape and render it like a definition list (similar to Parameters) instead of a single block of text.
|
Note that these changes does not fix #1171 |
I agree and will update the PR description for now. I will continue working on this and ask where I might get stuck or what needs approval for the approach. For now, I am exploring ways to avoid modifying too many files, which requires further discussion about the feature. |
|
This issue might be too big to tackle as a first issue. Since you started working on this I've made a note about this in the issue. If there's any particular area that you are more interested in I can help you find (or create a new) better suited first issue for you to work instead. |
Thank you for the clarification. Then I would like to enhance the experimental documentation feature, as I have made a proposal for the same. Can I DM you in the Slack Channel for guidance? |
Support documenting tuple return value elements individually
Fixes #1171
Bug/issue #, if applicable: #1171
Summary
DocC can now present named tuple return value documentation in the same way as Parameters: when you use the outline form under
- Returns:(nested list items like- name: description), the rendered page shows a definition list (term + description per element) instead of a single block of text. Existing documentation is unchanged: single-paragraph- Returns:and “all elements together” continue to work as before.Implementation (minimal, no new public API):
- Returns:has a nested list where each item is- name: description, we store oneReturnwhosecontentsis that innerUnorderedList.No new types;
ReturnsSectionandReturnare unchanged.privateso the Returns extension can reuse them for the outline form.Tuple-count validation (e.g. warn when documented elements don’t match the function’s return type) is left as a possible follow-up.
Dependencies
None.
Testing
Automated tests confirm that
- Returns:with a nested list is parsed as one Return containing an UnorderedList with quotient: and remainder: items. Existing tests still pass; run./bin/testorswift test --filter ListItemExtractorTests.For manual testing: build docc, create a small Swift package with a tuple-returning function documented using the outline form, generate docs with swift package generate-documentation, and open the symbol page.
The Return Value section should render a definition list for quotient and remainder.
Also verify that a simple
- Returns:paragraph still renders normally.Checklist
./bin/testscript and it succeededI am not sure which documentation needs to be updated