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

Populate the "Possible Values" section based on OAS data #857

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pdwilson12
Copy link
Contributor

@pdwilson12 pdwilson12 commented Mar 15, 2024

Bug/issue #, if applicable: rdar://123262314

Summary

REST/JSON data types support defining the allowed set of values a type can take (ie, an enumerated type) using the allowedValues key. These changes add support for generating a "Possible Values" section type for documenting the individual values, using a - PossibleValues: group in markdown. The rendered values will be limited to values defined by the symbol graph and will be presented in the same order as in the allowedValues key for the symbol.

Dependencies

None

Testing

Run DocC on the DictionaryData.docc fixture within the repo. The Month symbol includes 3 enumerated values and its markdown file attempts to document 2 of them out of order along with an invalid third entry. When rendered the real 3 values should be rendered in proper order along with the documentation for 2 of them.

Steps:

  1. Build docc for this branch
  2. Preview DictionaryData.docc and review (and edit) the Month page.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary (n/a)

@pdwilson12
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

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

I have some questions about the API design of the added public types.

Also, we should add tests that verify that diagnostics work and are associated with the content's file and source range. For example, it would be good the verify that a <doc:ThisWillNotResolve> raises a warning and highlights the right line of code when used in a "Possible Value" description.

@@ -106,6 +114,13 @@ extension Sequence where Element == InlineMarkup {
return nil
}

func extractPossibleValue() -> PossibleValue? {
if let (value, _, content, _) = splitNameAndContent() {
Copy link
Contributor

Choose a reason for hiding this comment

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

New code should capture and use the nameRange and itemRange. Otherwise diagnostics for this content can't be associated with the file where the content comes from.

It would also be good to add a test that verifies that diagnostics in the possible value content are presented with the relevant source and range information.

import SymbolKit

/// Documentation about the possible values for a symbol.
public struct PossibleValue {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems only relevant within PossibleValuesSection. Is it possible to make it a nested type instead of adding a top-level type with such a general name?

Comment on lines +65 to +67
while initialText[spaceIndex] == " " {
spaceIndex = initialText.index(spaceIndex, offsetBy: 1)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Directly manipulating a string index and using it to access the content in a loop like this is a bad way of working with Strings in Swift. It's possibly accidentally quadratic because the characters are not known to be the same number of bytes.

I think this is trying to do something like the below to find the start of the next if there are multiple spaces between the colon and the word

let nameStartIndex = initialText[...colonIndex].drop(while: { $0 != " " }).dropFirst().firstIndex(where: { $0 != " " }) 
    ?? initialText.startIndex

in which case it might be more robust to use $0.isWhitespace to also handle tabs and other whitespace characters.

let nameStartIndex = initialText[...colonIndex].drop(while: { !$0.isWhitespace }).dropFirst().firstIndex(where: { !$0.isWhitespace }) 
    ?? initialText.startIndex

Regardless. It would be good to add a short comment to describe why this is happening.

@@ -49,7 +49,7 @@ let simpleListItemTags = [
]

extension Sequence where Element == InlineMarkup {
private func splitNameAndContent() -> (name: String, nameRange: SourceRange?, content: [Markup], range: SourceRange?)? {
private func splitNameAndContent(skipStartingTag: Bool = false) -> (name: String, nameRange: SourceRange?, content: [Markup], range: SourceRange?)? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why is this parameter needed? If the code scans up to the first colon then does it matter if the line looks like

- one two: Description 

or

- one: Description 

?

Comment on lines +20 to +23
public var documentedValues: [PossibleValue]

/// The list of defined values for the symbol.
public var definedValues: [SymbolGraph.AnyScalar]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the structure of this type. Is there any relation between the documentedValues and definedValues? It looks to me like each documented values's value may(?) correspond to one of the defined values? If that's correct, would it be better to model this like [SymbolGraph.AnyScalar: [Markup]?] or something along those lines?

It's hard and takes a long time to change a public API after it's been added. If we aren't sure that we have the right API then we shouldn't make it public yet.

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.

None yet

2 participants