-
Notifications
You must be signed in to change notification settings - Fork 488
Documentation for Switch, Return, Throw and Type syntax nodes #3231
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
base: main
Are you sure you want to change the base?
Documentation for Switch, Return, Throw and Type syntax nodes #3231
Conversation
ahoppen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the documentation @Padmashree06. I added a few comments below.
| base: .syntax, | ||
| nameForDiagnostics: nil, | ||
| documentation: """ | ||
| A clause that specifies the underlying type of a type declaration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| A clause that specifies the underlying type of a type declaration. | |
| A clause that specifies the underlying type of a `typealias` or `associatedtype` declaration. |
| ``` | ||
|
|
||
| ```swift | ||
| func greet(name: String) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function parameters actually don’t use TypeAnnotationSyntax, see https://swiftpackageindex.com/swiftlang/swift-syntax/main/documentation/swiftsyntax/functionparametersyntax
| A statement that exits a function or closure and optionally returns a value. | ||
|
|
||
| Written as: | ||
| ```swift | ||
| return | ||
| ``` | ||
|
|
||
| ```swift | ||
| return <expr> | ||
| ``` | ||
| """, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation seems a little off to me here, both within the code blocks and overall.
Same below
| name: "expression", | ||
| kind: .node(kind: .expr) | ||
| kind: .node(kind: .expr), | ||
| nameForDiagnostics: "error" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intentionally change the name for diagnostics here?
rintaro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this issue!
I left a few comment regarding the general direction of this. I'd like to hear the opinions from @ahoppen about them as well.
| base: .syntax, | ||
| nameForDiagnostics: nil, | ||
| documentation: """ | ||
| A clause that specifies the return type of a function or closure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually function, subscript, type, or closure.
// function
func f() -> Int { }
// subscript
subscript(I: Int) -> Int {}
// type
let a: () -> Int = { 1 }
// closure
_ = { () -> Int in 1 }But I'm not sure we should list everything here. This information is automatically rendered as "Contained in" section.
So maybe A clause that specifies the return type is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My proposal would be A clause that specifies the return type, typically of a function or subscript. That leaves room for the slightly more edge cases in the Contained In section, while still giving you an idea of what this is about.
| Written as: | ||
| ```swift | ||
| -> <type> | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This technically a duplicated information with ### Children section in the rendered doc-comments. Although this simplified code might be nice to read, my personal preference is not to have this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, I don’t have a strong opinion here either way
| -> <type> | ||
| ``` | ||
|
|
||
| ### Examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure we should write a section (###) here. It's the same header level as other generated sections (i.e. ### Children and ### Contained in), If we want another section for "examples", we might need another property on Node.
But I think something like this would be enough.
A clause that specifies the return type.
For example, the `-> Int` part in
```swift
func foo() throws -> Int { ... }
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a lot of precedence for ### Examples sections in the documentation, just search for that string in CodeGeneration. So, I vote to keep it.
I’m not opposed to moving the examples into a different property of Node but I think that should be a different PR.
|
The requested changes have been addressed, and the test suite passes without issues. |
ahoppen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you
This PR adds documentation comments for the following Syntax nodes:
SwitchCaseLabelSyntaxSwitchDefaultLabelSyntaxSwitchCaseSyntaxThrowStmtSyntaxReturnClauseSyntaxReturnStmtSyntaxTypeInitializerClauseSyntaxTypeAnnotationSyntaxThe documentation follows the format used in #1527 and includes
brief explanations and examples where applicable.
This contributes to issue #1528.