Skip to content

Conversation

@ahoppen
Copy link
Member

@ahoppen ahoppen commented Aug 3, 2023

The document was a little bit outdated. I simplified it to focus on the common development workflows that just open swift-syntax as a SwiftPM package, skipping all paths that go through build-script.py, which shouldn’t be necessary for 95% of all contributors, especially newcomers who this document is designed for.

@ahoppen ahoppen requested a review from bnbarham August 3, 2023 14:55
@ahoppen
Copy link
Member Author

ahoppen commented Aug 3, 2023

@swift-ci Please test

@Matejkob
Copy link
Contributor

Matejkob commented Aug 3, 2023

Speaking of documentation covering contributing to the project, recently I went through all the articles from the "Contributing" section in DocC documentation for SwiftSyntax. If I'm not mistaken, it also contains some outdated documentation. For instance, this one still mentions code from python-time. I wonder if we updated this one; maybe it would be a good time to also take a look at the DocC one?

@ahoppen
Copy link
Member Author

ahoppen commented Aug 3, 2023

Thanks for pointing it out, @Matejkob. I updated ChangingSwiftSyntax.md as well. If you find any inconsistencies in the documentation, I encourage you to file issues for them or, even better, to open a PR to update the documentation.

@ahoppen
Copy link
Member Author

ahoppen commented Aug 3, 2023

@swift-ci Please test

Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

Thanks for updating!

CONTRIBUTING.md Outdated
## Building & Testing

## Building
To build swift-syntax, open the package in Xcode, Visual Studio Code with the [Swift for Visual Studio Code](https://github.com/swift-server/vscode-swift) installed, or open it with your favorite text editor and build from the command line using `swift build`.
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should call out that it's just a package, something like:

swift-syntax is a SwiftPM package, so you can build and test it using anything that supports packages - opening in Xcode, Visual Studio Code with ..., or through the command line using `swift build` and `swift test`.

Then skipping the You can run... below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea

CONTRIBUTING.md Outdated
SwiftSyntax is being formatted using [swift-format](http://github.com/apple/swift-format) to ensure a consistent style.
swift-syntax is formatted using [swift-format](http://github.com/apple/swift-format) to ensure a consistent style.

To format your changes run `format.py` at the root of this repository. If you have a `swift-format` executable ready, you can pass it to `format.py`. If you do not, `format.py` will build its own copy of `swift-format` in /tmp/swift-format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To format your changes run `format.py` at the root of this repository. If you have a `swift-format` executable ready, you can pass it to `format.py`. If you do not, `format.py` will build its own copy of `swift-format` in /tmp/swift-format.
To format your changes run `format.py` at the root of this repository. If you have a `swift-format` executable ready, you can pass it to `format.py`. If you do not, `format.py` will build its own copy of `swift-format` in `/tmp/swift-format`.

not only identifies it uniquely to Swift Syntax, it also provides a handle that
other nodes can use to refer to it. The kind of a syntax node defines the
class of syntax the node belongs to. All nodes are at least of the `Syntax`
node consists of a kind, which also defines the node’s name, a base kind, and a list of
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
node consists of a kind, which also defines the node’s name, a base kind, and a list of
node consists of at least a kind (which also defines the node’s name), a base kind, and a list of child nodes

Is nameForDiagnostics required? If so maybe add that to the list as well. And do you think it would be useful to add a trailing comment to each below, or the documentation in Node is enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

nameForDiagnostics is not required. If nil it will use the nameForDiagnostics of the parent node.

I think the documentation of Node is enough. If we duplicate it and change it in the future, we will just forget to update this document again.

@Matejkob
Copy link
Contributor

Matejkob commented Aug 3, 2023

Thanks for pointing it out, @Matejkob. I updated ChangingSwiftSyntax.md as well. If you find any inconsistencies in the documentation, I encourage you to file issues for them or, even better, to open a PR to update the documentation.

My bad for not filing an issue right away. 🙈 I'll make sure to do it properly next time.

@ahoppen
Copy link
Member Author

ahoppen commented Aug 3, 2023

My bad for not filing an issue right away. 🙈 I'll make sure to do it properly next time.

Not a problem. I just wanted to say: Don’t be shy to file issues or PRs about it.

ahoppen added 2 commits August 3, 2023 09:38
The document was a little bit outdated. I simplified it to focus on the common development workflows that just open swift-syntax as a SwiftPM package, skipping all paths that go through build-script.py, which shouldn’t be necessary for 95% of all contributors, especially newcomers who this document is designed for.
Update some outdated comments from ChangingSwiftSyntax.md
@ahoppen ahoppen force-pushed the ahoppen/update-contributing branch from a4c9e7e to e8fc865 Compare August 3, 2023 16:38
@ahoppen
Copy link
Member Author

ahoppen commented Aug 3, 2023

@swift-ci Please test

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Aug 3, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Aug 3, 2023

@swift-ci Please test macOS

@kimdv
Copy link
Contributor

kimdv commented Aug 4, 2023

Test failed with

Failed Tests (1):
  Swift(macosx-x86_64) :: stdlib/Observation/ObservableAvailabilityCycle.swift

looked a bit unrelated

@kimdv
Copy link
Contributor

kimdv commented Aug 4, 2023

@swift-ci Please test macOS

1 similar comment
@ahoppen
Copy link
Member Author

ahoppen commented Aug 4, 2023

@swift-ci Please test macOS

@ahoppen ahoppen merged commit f4dd574 into swiftlang:main Aug 5, 2023
@ahoppen ahoppen deleted the ahoppen/update-contributing branch August 5, 2023 03:47
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.

4 participants