Skip to content

Add an 'indented' method to SyntaxProtocol #2843

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 12 commits into from
Oct 30, 2024
Merged

Add an 'indented' method to SyntaxProtocol #2843

merged 12 commits into from
Oct 30, 2024

Conversation

j-f1
Copy link
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
Contributor Author

j-f1 commented Sep 7, 2024

@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.

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.

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

This comment was marked as outdated.

@j-f1
Copy link
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
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
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
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.

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 you for coming back to this after I took a while, @j-f1. Looks good to me, just a few minor comments.

self.shouldIndent = indentFirstLine
}

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

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
Contributor Author

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.

1 similar comment
@j-f1

This comment was marked as outdated.

@ahoppen ahoppen enabled auto-merge October 29, 2024 00:33
@j-f1

This comment was marked as outdated.

1 similar comment
@j-f1

This comment was marked as outdated.

@j-f1

This comment was marked as outdated.

@j-f1
Copy link
Contributor Author

j-f1 commented Oct 30, 2024

@swift-ci please test

@j-f1
Copy link
Contributor Author

j-f1 commented Oct 30, 2024

@swift-ci please test Windows

@ahoppen ahoppen merged commit 91444aa into main Oct 30, 2024
3 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants