Skip to content

Make the first character of both name and deprecatedName in every Child lowercase #2055

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 15, 2023
Merged

Make the first character of both name and deprecatedName in every Child lowercase #2055

merged 2 commits into from
Aug 15, 2023

Conversation

Matejkob
Copy link
Contributor

Resolves #1901

@Matejkob Matejkob requested a review from ahoppen as a code owner August 12, 2023 10:03
Matejkob
Matejkob commented Aug 12, 2023
View reviewed changes
CodeGeneration/Sources/SyntaxSupport/DeclNodes.swift Outdated
kind: .collection(kind: .codeBlockItemList, collectionElementName: "Statement")
),
Child(
name: "EndOfFileToken",
name: "endOfFileToken",
deprecatedName: "EOFToken",
Copy link
Contributor Author

@Matejkob Matejkob Aug 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm uncertain about the optimal way to handle this edge case.

If we want to make this deprecatedName lowercase as well, I'm not sure there's a better solution than simply hardcoding the condition if deprecatedName == "eofToken" { ... somewhere 😕

Copy link
Member

Choose a reason for hiding this comment

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

I’m sorry, I completely missed this comment. I haven’t tried, but what I would expect is that we should be able to change deprecatedName to eofToken here. AFAICT this should only break the compatibility layer of SourceFileSyntax.unexpectedAfterEOFToken and SourceFileSyntax.unexpectedBetweenStatementsAndEOFToken, which, I believe is acceptable. We are making source breaking changes to swift-syntax occasionally (like making properties non-optional) and I don’t expect many clients to access these two properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't suggesting this approach because I was thinking that we don't want to break the capability layer. However, if you're saying that we can do such changes, then I'm on board.

Here is the exact diff in the generated files:

diff --git a/Sources/SwiftSyntax/generated/RenamedChildrenCompatibility.swift b/Sources/SwiftSyntax/generated/RenamedChildrenCompatibility.swift
index 2df170c1..972b1377 100644
--- a/Sources/SwiftSyntax/generated/RenamedChildrenCompatibility.swift
+++ b/Sources/SwiftSyntax/generated/RenamedChildrenCompatibility.swift
@@ -7027,7 +7027,7 @@ extension SomeOrAnyTypeSyntax {

 extension SourceFileSyntax {
   @available(*, deprecated, renamed: "unexpectedBetweenStatementsAndEndOfFileToken")
-  public var unexpectedBetweenStatementsAndEOFToken: UnexpectedNodesSyntax? {
+  public var unexpectedBetweenStatementsAndEofToken: UnexpectedNodesSyntax? {
     get {
       return unexpectedBetweenStatementsAndEndOfFileToken
     }
@@ -7047,7 +7047,7 @@ extension SourceFileSyntax {
   }

   @available(*, deprecated, renamed: "unexpectedAfterEndOfFileToken")
-  public var unexpectedAfterEOFToken: UnexpectedNodesSyntax? {
+  public var unexpectedAfterEofToken: UnexpectedNodesSyntax? {
     get {
       return unexpectedAfterEndOfFileToken
     }
@@ -7062,9 +7062,9 @@ extension SourceFileSyntax {
       leadingTrivia: Trivia? = nil,
       _ unexpectedBeforeStatements: UnexpectedNodesSyntax? = nil,
       statements: CodeBlockItemListSyntax,
-      _ unexpectedBetweenStatementsAndEOFToken: UnexpectedNodesSyntax? = nil,
+      _ unexpectedBetweenStatementsAndEofToken: UnexpectedNodesSyntax? = nil,
       eofToken: TokenSyntax = .endOfFileToken(),
-      _ unexpectedAfterEOFToken: UnexpectedNodesSyntax? = nil,
+      _ unexpectedAfterEofToken: UnexpectedNodesSyntax? = nil,
       trailingTrivia: Trivia? = nil

   ) {
@@ -7072,9 +7072,9 @@ extension SourceFileSyntax {
         leadingTrivia: leadingTrivia,
         leadingTrivia: leadingTrivia,
         unexpectedBeforeStatements,
         statements: statements,
-        unexpectedBetweenStatementsAndEOFToken,
+        unexpectedBetweenStatementsAndEofToken,
         endOfFileToken: eofToken,
-        unexpectedAfterEOFToken,
+        unexpectedAfterEofToken,
         trailingTrivia: trailingTrivia
       )
   }
diff --git a/Sources/SwiftSyntaxBuilder/generated/RenamedChildrenBuilderCompatibility.swift b/Sources/SwiftSyntaxBuilder/generated/RenamedChildrenBuil
derCompatibility.swift
index ecaee9bc..53132ddb 100644
--- a/Sources/SwiftSyntaxBuilder/generated/RenamedChildrenBuilderCompatibility.swift
+++ b/Sources/SwiftSyntaxBuilder/generated/RenamedChildrenBuilderCompatibility.swift
@@ -751,9 +751,9 @@ extension SourceFileSyntax {
   public init(
       leadingTrivia: Trivia? = nil,
       unexpectedBeforeStatements: UnexpectedNodesSyntax? = nil,
-      unexpectedBetweenStatementsAndEOFToken: UnexpectedNodesSyntax? = nil,
+      unexpectedBetweenStatementsAndEofToken: UnexpectedNodesSyntax? = nil,
       eofToken: TokenSyntax = .endOfFileToken(),
-      unexpectedAfterEOFToken: UnexpectedNodesSyntax? = nil,
+      unexpectedAfterEofToken: UnexpectedNodesSyntax? = nil,
       @CodeBlockItemListBuilder statementsBuilder: () throws -> CodeBlockItemListSyntax,
       trailingTrivia: Trivia? = nil
     ) rethrows {
@@ -761,9 +761,9 @@ extension SourceFileSyntax {
         leadingTrivia: leadingTrivia,
         unexpectedBeforeStatements,
         statements: statementsBuilder(),
-        unexpectedBetweenStatementsAndEOFToken,
+        unexpectedBetweenStatementsAndEofToken,
         endOfFileToken: eofToken,
-        unexpectedAfterEOFToken,
+        unexpectedAfterEofToken,
         trailingTrivia: trailingTrivia
       )
   }

Is EofToken acceptable?

Copy link
Member

@ahoppen ahoppen Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that’s acceptable. I don’t expect this to affect anyone really and it cleans up the internal implementation.

Matejkob
Matejkob commented Aug 12, 2023
View reviewed changes
CodeGeneration/Sources/SyntaxSupport/Traits.swift Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also wondering if we should change Childs names for Traits, but I actually don't see any reasons why not.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree. We should change them as well.

ahoppen
ahoppen reviewed Aug 14, 2023
View reviewed changes
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 @Matejkob . This is a lot more intuitive.

CodeGeneration/Sources/SyntaxSupport/Child.swift
@@ -214,8 +214,7 @@ public class Child {
documentation: String? = nil,
isOptional: Bool = false
) {
precondition(name.first?.isUppercase ?? true, "The first letter of a child’s name should be uppercase")
precondition(deprecatedName?.first?.isUppercase ?? true, "The first letter of a child’s name should be uppercase")
precondition(name.first?.isLowercase ?? true, "The first letter of a child’s name should be lowercase")
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also precondition that deprecatedName starts with a lowercase letter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, but before selecting the exact implementation, I would like to discuss the best option. This is why I've added this comment:

I'm uncertain about the optimal way to handle this edge case.

If we want to make deprecatedName lowercase as well, I'm not sure there's a better solution than simply hardcoding the condition if deprecatedName == "eofToken" { ... somewhere 😕

Sorry if it wasn't understandable in the first place.

So, we are dealing with an edge case where deprecatedName is set to "EOFToken", and I'm wondering what would be the best approach to address this situation. The best solution that comes to my mind is hardcoding the condition if deprecatedName == "eofToken" { ... somewhere... :/

@Matejkob Matejkob force-pushed the lowercase-first-chart-of-child-names branch from e163544 to e5b1a2b Compare August 14, 2023 20:12
@ahoppen
Copy link
Member

ahoppen commented Aug 14, 2023

@Matejkob There is a merge conflict. Could you rebase your PR?

@Matejkob
Make first characters of all Child's name in CodeGeneration lowercase
9f8bdaa 
Adjusted the 'name' property of Child to begin with a lowercase character for consistency and intuition, without altering the generated code's output.
@Matejkob
Make first characters of all Child's deprecatedName in CodeGeneration…
33e8107 
… lowercase

Adjusted the 'deprecatedName' property of Child to begin with a lowercase character for consistency and intuition, without altering the generated code's output.
@Matejkob Matejkob force-pushed the lowercase-first-chart-of-child-names branch from e5b1a2b to 33e8107 Compare August 15, 2023 16:29
@Matejkob
Copy link
Contributor Author

@ahoppen I've just solved the merge conflict. Thank you.

@ahoppen
Copy link
Member

ahoppen commented Aug 15, 2023

Let’s try to quickly get it in before there’s another conflict.

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge August 15, 2023 16:33
@ahoppen
Copy link
Member

ahoppen commented Aug 15, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 2e3c42c into swiftlang:main Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change name property of Child in CodeGeneration to be lowercase
2 participants
@Matejkob @ahoppen