Skip to content

Improve implementation of BasicFormat (yet again) #1558

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 3 commits into from
Apr 21, 2023
Merged

Improve implementation of BasicFormat (yet again) #1558

merged 3 commits into from
Apr 21, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Apr 19, 2023

This re-writes the implementation of BasicFormat once again to better handle user-indented code.

@ahoppen ahoppen requested a review from bnbarham April 19, 2023 00:56
@ahoppen ahoppen force-pushed the ahoppen/update-basic-format branch from e759209 to 23d2072 Compare April 19, 2023 01:16
@ahoppen
Copy link
Member Author

ahoppen commented Apr 19, 2023

@swift-ci Please test

kimdv
kimdv reviewed Apr 19, 2023
View reviewed changes
kimdv
kimdv reviewed Apr 19, 2023
View reviewed changes
kimdv
kimdv reviewed Apr 19, 2023
View reviewed changes
Sources/SwiftBasicFormat/BasicFormat.swift Outdated
/// This is used as a reference-point to indent user-indented code.
var anchorPoints: [TokenSyntax: Trivia] = [:]

public init(indentationIncrement: Trivia = .spaces(4), initialIndentation: Trivia = []) {
Copy link
Contributor

@kimdv kimdv Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if I like the default value is []. I think it's a but hard to understand maybe for new developers.

But on the the other hand I cannot find any other name than .zero, .none, .empty. And I'm not convinced they fit either.

Specially because we on line 36 add and array in an array.
That is the same if we did initialIndentation: Trivia = .space(2) etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

We used to have Trivia.zero but IIRC @bnbarham removed it because it was redundant with the empty array. Do you have strong options on whether we should add it (or .empty) back in again?

In line 36: Well, indentationStack is an array of trivia again. And arrays of arrays aren’t that uncommon, so I think that should be fine…

kimdv
kimdv reviewed Apr 19, 2023
View reviewed changes
kimdv
kimdv approved these changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@kimdv kimdv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is looking really great! 😍

Huge improvement IMO.

Just added some comments for discussion/questions I have.

bnbarham
bnbarham reviewed Apr 19, 2023
View reviewed changes
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.

Nice, seems to work pretty well. Might be good to add a stripIndentation so the BasicFormat indentation can be preferred even if the string interpolation is being used with whatever indentation an author had there.

@ahoppen ahoppen force-pushed the ahoppen/update-basic-format branch 2 times, most recently from 192a551 to 20e2ebe Compare April 20, 2023 00:24
ahoppen and others added 2 commits April 19, 2023 17:28
@ahoppen @bnbarham
Improve implementation of BasicFormat (yet again)
4a3f0c0 
This re-writes the implementation of BasicFormat once again to better handle user-indented code.

Co-authored-by: Ben Barham <[email protected]>
@ahoppen
Improve performance of BasicFormat
1a555cf 
This mostly improves performance by calling `previousToken`, `nextToken`, and `firstToken` less often.
@ahoppen ahoppen force-pushed the ahoppen/update-basic-format branch from 20e2ebe to 46aab36 Compare April 20, 2023 01:32
@ahoppen
Copy link
Member Author

ahoppen commented Apr 20, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Apr 20, 2023

@swift-ci Please test Windows

@ahoppen ahoppen force-pushed the ahoppen/update-basic-format branch 2 times, most recently from 684d268 to 46aab36 Compare April 20, 2023 01:43
@ahoppen
Copy link
Member Author

ahoppen commented Apr 20, 2023

@swift-ci Please test

@ahoppen ahoppen mentioned this pull request Apr 20, 2023
Merged
@ahoppen
Copy link
Member Author

ahoppen commented Apr 20, 2023

@swift-ci Please test Linux

@ahoppen ahoppen mentioned this pull request Apr 20, 2023
Merged
bnbarham
bnbarham reviewed Apr 20, 2023
View reviewed changes
@ahoppen ahoppen force-pushed the ahoppen/update-basic-format branch from 46aab36 to 2e4761e Compare April 20, 2023 17:01
@ahoppen
Copy link
Member Author

ahoppen commented Apr 20, 2023

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Apr 21, 2023

@swift-ci Please test Windows

@kimdv kimdv mentioned this pull request Apr 21, 2023
Merged
@ahoppen ahoppen merged commit c7087ad into swiftlang:main Apr 21, 2023
@ahoppen ahoppen deleted the ahoppen/update-basic-format branch April 22, 2023 14:45
@ahoppen ahoppen mentioned this pull request Apr 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnbarham bnbarham bnbarham left review comments

@kimdv kimdv kimdv 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.
3 participants
@ahoppen @bnbarham @kimdv