Skip to content

Add an 'indented' method to SyntaxProtocol#2843

Merged
ahoppen merged 12 commits intomainfrom
jed/indented
Oct 30, 2024
Merged

Add an 'indented' method to SyntaxProtocol#2843
ahoppen merged 12 commits intomainfrom
jed/indented

Conversation

@j-f1
Copy link
Copy Markdown
Contributor

@j-f1 j-f1 commented Sep 7, 2024

This method allows for easily indenting a node tree without needing to manually find where to place the new trivia.

I also chose not to implement a version that automatically detects an amount to indent by using BasicFormat.inferIndentation since that would probably produce better results if called higher up in the tree, and the user of this API can more easily cache that calculated indentation amount and pass it to indent multiple times as needed.

@j-f1 j-f1 changed the title Add an 'indented method to SyntaxProtocol Add an 'indented' method to SyntaxProtocol Sep 7, 2024
@j-f1
Copy link
Copy Markdown
Contributor Author

j-f1 commented Sep 7, 2024

@swift-ci please test

ahoppen
ahoppen reviewed Oct 23, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Sorry for taking sooooooo long to get back to this. We already have Indenter in SwiftSyntaxBuilder, which serves the same purpose. I don’t think that Indenter was ever intended to be public API, but now it’s out there.

I think your API is the nicer one (doesn’t expose a SyntaxRewriter) but I think it would be good to unify these two implementations. Could you:

  • Mark the Indenter class in SwiftSyntaxBuilder as deprecated?
  • Migrate all uses of Indenter inside swift-syntax to your indentation method?
  • Add release note entries for the deprecation of the Indenter and the introduction of the new indented method?

@j-f1

This comment was marked as outdated.

Sign in to view
@j-f1 j-f1 requested a review from ahoppen October 26, 2024 22:58
@j-f1

This comment was marked as outdated.

Sign in to view
@j-f1
Copy link
Copy Markdown
Contributor Author

j-f1 commented Oct 26, 2024

Why might the indented(by:) method not be able to be found? Things work fine for me locally.

/Users/ec2-user/jenkins/workspace/swift-syntax-PR-macOS/branch-main/swift-syntax/Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift:85:27: error: value of type 'Node' has no member 'indented'
      indentedNode = node.indented(by: lastIndentation)
                     ~~~~ ^~~~~~~~

@CodaFi
Copy link
Copy Markdown
Contributor

CodaFi commented Oct 27, 2024

A Node is not a kind of Syntax, it’s meta-information about the declaration of a syntax node the builder will spit out. You may want to revert your change in that file.

@j-f1
Copy link
Copy Markdown
Contributor Author

j-f1 commented Oct 28, 2024

It’s a local generic type (which must be shadowing the real Node type): func appendInterpolation<Node: SyntaxProtocol>.

@ahoppen
Copy link
Copy Markdown
Member

ahoppen commented Oct 28, 2024

Why might the indented(by:) method not be able to be found? Things work fine for me locally.

/Users/ec2-user/jenkins/workspace/swift-syntax-PR-macOS/branch-main/swift-syntax/Sources/SwiftSyntaxBuilder/Syntax+StringInterpolation.swift:85:27: error: value of type 'Node' has no member 'indented'
      indentedNode = node.indented(by: lastIndentation)
                     ~~~~ ^~~~~~~~

You need to add Indenter.swift to CMakeLists.txt so we can find it when building swift-syntax using CMake for the compiler.

ahoppen
ahoppen reviewed Oct 29, 2024
View reviewed changes
Copy link
Copy Markdown
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 you for coming back to this after I took a while, @j-f1. Looks good to me, just a few minor comments.

Sources/SwiftBasicFormat/Indenter.swift Outdated
self.shouldIndent = indentFirstLine
}

private func indentIfNeeded() -> [TriviaPiece] {
Copy link
Copy Markdown
Member

@ahoppen ahoppen Oct 28, 2024

Choose a reason for hiding this comment

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

Also, I would call it indentationIfNeeded because indentIfNeeded sounds like an action that’s actively indenting something but this just returns the indentation.

Same for indentAfterNewlines.

Copy link
Copy Markdown
Contributor Author

@j-f1 j-f1 Oct 29, 2024

Choose a reason for hiding this comment

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

indentAfterNewlines does actually perform the indentation (and returns the indented string) but I’ve renamed indentIfNeeded.

@j-f1

This comment was marked as outdated.

Sign in to view
1 similar comment
@j-f1

This comment was marked as outdated.

Sign in to view
@ahoppen ahoppen enabled auto-merge October 29, 2024 00:33
@j-f1

This comment was marked as outdated.

Sign in to view
1 similar comment
@j-f1

This comment was marked as outdated.

Sign in to view
@j-f1

This comment was marked as outdated.

Sign in to view
@j-f1
Copy link
Copy Markdown
Contributor Author

j-f1 commented Oct 30, 2024

@swift-ci please test

@j-f1
Copy link
Copy Markdown
Contributor Author

j-f1 commented Oct 30, 2024

@swift-ci please test Windows

@ahoppen ahoppen merged commit 91444aa into main Oct 30, 2024
@ahoppen ahoppen deleted the jed/indented branch October 30, 2024 20:26
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

@bnbarham bnbarham Awaiting requested review from bnbarham

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@j-f1 @CodaFi @ahoppen