Skip to content

Documentation - Typos & Consistency Fixes #1841

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

Merged
merged 1 commit into from
Jul 8, 2023
Merged

Documentation - Typos & Consistency Fixes #1841

merged 1 commit into from
Jul 8, 2023

Conversation

joey-gm
Copy link
Contributor

@joey-gm joey-gm commented Jun 25, 2023

Fixes some typos and formatting inconsistencies.

Copy link
Contributor

@kimdv kimdv left a comment

Choose a reason for hiding this comment

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

Hi @joey-gm and welcome to SwiftSyntax! 🥳

Thanks for going trough the documentation and typos.
I see that you have changed some files in the generated files.
Those files are generated automatically by the Codegenerator.

Those files should be changed in the corresponding template file.
After that you navigate inside CodeGeneration folder and run swift run generate-swiftsyntax ../Sources

This will ensure that the generated code at update correctly.
If there is any question feel free to comment on this PR and I will gladly help you.

@@ -1575,8 +1575,7 @@ public struct MissingStmtSyntax: StmtSyntaxProtocol, SyntaxHashable {
}
}

/// A placeholder, i.e. `<#statement#>` that can be inserted into the source code to represent the missing pattern.
/// This token should always have `presence = .missing`.
/// A placeholder, i.e. `<#statement#>` that can be inserted into the source code to represent the missing pattern./// This token should always have `presence = .missing`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like CodeGeneration fails to add a proper line break for this particular comment.

I tried various line break options (e.g. extra spaces) in the codegen file below without avail.

https://github.com/apple/swift-syntax/blob/5922b69d0db36e5ebdadfe7a893f00ca576cb2e6/CodeGeneration/Sources/SyntaxSupport/CommonNodes.swift#L234

@joey-gm
Copy link
Contributor Author

joey-gm commented Jun 26, 2023

@kimdv Thanks for the guidance and instruction!

I have fixed the typos in the corresponding template files under CodeGeneration, and have re-generated the swift codes. All files should now be in sync.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you for fixing all those typos @joey-gm. I’ve got a couple of comments inline, otherwise it looks good to me.

@joey-gm
Copy link
Contributor Author

joey-gm commented Jun 26, 2023

Thank you for fixing all those typos @joey-gm. I’ve got a couple of comments inline, otherwise it looks good to me.

Thanks! Incorporated your suggestions.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks. Two more comments.

Could you also squash your commits? It just makes for a nicer git history.

/// The enum describes how the SyntaxVistor should continue after visiting
/// The enum describes how the SyntaxVisitor should continue after visiting
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think you re-generated SyntaxVisitor. If you did, this line would be

/// The enum describes how the ``SyntaxVisitor`` should continue after visiting

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

One more squash of these two commits and we’re ready to merge.

@joey-gm
Copy link
Contributor Author

joey-gm commented Jun 26, 2023

One more squash of these two commits and we’re ready to merge.

All done. Thanks!

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks. Once CI passes, I’ll merge the PR

@ahoppen ahoppen enabled auto-merge June 26, 2023 14:55
@ahoppen
Copy link
Member

ahoppen commented Jun 26, 2023

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Jun 27, 2023

@joey-gm Unfortunately we’ve got a merge conflict now. Could you rebase your changes on top of main?

The CI failures are because we need to update a test case in the apple/swift repo, which I’m doing in swiftlang/swift#66950.

auto-merge was automatically disabled June 27, 2023 08:16

Head branch was pushed to by a user without write access

@ahoppen
Copy link
Member

ahoppen commented Jun 27, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge June 27, 2023 08:24
@joey-gm
Copy link
Contributor Author

joey-gm commented Jun 27, 2023

Hi Alex @ahoppen,

I've rebased and recommitted the changes.

Below are the steps I’ve taken. Could you please review and make sure I’ve rebased properly? (I’m an amateur and not too familiar with git’s branch management. I skimmed through the changes in the latest commit and they look okay.)

Thanks!

(Sync my main branch with the Apple's main)
git checkout documentation // documentation is the branch which I made the changes locally
git rebase main

(Regenerate codes)
cd CodeGeneration
swift run generate-swiftsyntax ../Sources

(Soft reset, commit, and push)
cd ..
git reset --soft main
git commit -m "Documentation - Typos & Consistency Fixes"
git push -f

@kimdv
Copy link
Contributor

kimdv commented Jun 27, 2023

@swift-ci please test windows

@ahoppen
Copy link
Member

ahoppen commented Jun 27, 2023

You’re rebase looks good, @joey-gm.

@ahoppen
Copy link
Member

ahoppen commented Jun 27, 2023

swiftlang/swift#66950

@swift-ci Please test

@ahoppen ahoppen disabled auto-merge June 27, 2023 11:17
@ahoppen
Copy link
Member

ahoppen commented Jun 27, 2023

CI is currently failing with an unrelated issue (#1855). I’ll trigger CI agains once that issue has been resolved.

@kimdv
Copy link
Contributor

kimdv commented Jun 27, 2023

swiftlang/swift#66950

@swift-ci Please test

1 similar comment
@kimdv
Copy link
Contributor

kimdv commented Jun 28, 2023

swiftlang/swift#66950

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Jun 28, 2023

A few tests in this repo failed, AFAICT mostly because of the “preceed” -> “precede” rename. Could you adjust those failing test cases?

@joey-gm
Copy link
Contributor Author

joey-gm commented Jun 28, 2023

A few tests in this repo failed, AFAICT mostly because of the “preceed” -> “precede” rename. Could you adjust those failing test cases?

Fixed the typos and also changed the "eofToken" terminology to "endOfFileToken" in Tests/SwiftSyntaxTest/DebugDescriptionTests.swift

Passed all the tests in my local branch (macOS)

@kimdv
Copy link
Contributor

kimdv commented Jun 28, 2023

A few tests in this repo failed, AFAICT mostly because of the “preceed” -> “precede” rename. Could you adjust those failing test cases?

Fixed the typos and also changed the "eofToken" terminology to "endOfFileToken" in Tests/SwiftSyntaxTest/DebugDescriptionTests.swift

Passed all the tests in my local branch (macOS)

Have a PR open for fixing the endOfFileToken in #1860

@kimdv
Copy link
Contributor

kimdv commented Jun 29, 2023

@joey-gm if you rebase this branch it should be fixed

@kimdv
Copy link
Contributor

kimdv commented Jun 29, 2023

@swift-ci please test

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thank you 🙏🏽

@ahoppen
Copy link
Member

ahoppen commented Jul 6, 2023

@swift-ci Please test

@ahoppen
Copy link
Member

ahoppen commented Jul 6, 2023

swiftlang/swift#66950

@swift-ci Please test macOS

@ahoppen
Copy link
Member

ahoppen commented Jul 7, 2023

I’m sorry @joey-gm, this PR conflicted with #1878. Could you rebase once more.

@joey-gm
Copy link
Contributor Author

joey-gm commented Jul 7, 2023

I’m sorry @joey-gm, this PR conflicted with #1878. Could you rebase once more.

@ahoppen Updated and rebased. Please kindly check and test if this resolves the conflict.

@ahoppen
Copy link
Member

ahoppen commented Jul 7, 2023

Thank you! Let’s trigger CI again.

@ahoppen
Copy link
Member

ahoppen commented Jul 7, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge July 7, 2023 12:48
@ahoppen
Copy link
Member

ahoppen commented Jul 7, 2023

@swift-ci Please test Windows

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

CI found one more inconsistency where you only updated a generated file. I hope that we can get this merged soon.

/// The arguments of the initializer. While the function signature allows specifying an return clause, doing so is not semantically valid.
/// The arguments of the initializer. While the function signature allows specifying a return clause, doing so is not semantically valid.
Copy link
Member

Choose a reason for hiding this comment

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

auto-merge was automatically disabled July 7, 2023 14:26

Head branch was pushed to by a user without write access

@@ -1353,7 +1353,7 @@ public let DECL_NODES: [Node] = [
Child(
name: "Body",
kind: .node(kind: .codeBlock),
documentation: "The initializer’s body. Missing if the initialier is a requirement of a protocol declaration.",
documentation: "The initializer’s body. Missing if the initializer is a requirement of a protocol declaration.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed one more typo and regenerated the codes

@joey-gm
Copy link
Contributor Author

joey-gm commented Jul 7, 2023

CI found one more inconsistency where you only updated a generated file. I hope that we can get this merged soon.

Updated. Please kindly test. :)

@ahoppen
Copy link
Member

ahoppen commented Jul 7, 2023

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge July 7, 2023 17:26
@ahoppen ahoppen disabled auto-merge July 7, 2023 20:58
@ahoppen
Copy link
Member

ahoppen commented Jul 7, 2023

swiftlang/swift#66950

@swift-ci Please test

@ahoppen ahoppen merged commit c808498 into swiftlang:main Jul 8, 2023
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