Skip to content
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

Formatter is not idempotent, keeps adding comments #2261

Closed
kubukoz opened this issue Apr 26, 2024 · 0 comments · Fixed by #2283
Closed

Formatter is not idempotent, keeps adding comments #2261

kubukoz opened this issue Apr 26, 2024 · 0 comments · Fixed by #2283

Comments

@kubukoz
Copy link
Contributor

kubukoz commented Apr 26, 2024

Smithy CLI version: 1.48.0

Input:

$version: "2"

namespace test

operation Hello {
    errors: [
        // HelloError
    ]
}

smithy-cli format foo.smithy

Result 1:

$version: "2"

namespace test

operation Hello {
    // HelloError
    errors: [
        // HelloError
    ]
}

Run it again, result 2:

$version: "2"

namespace test

operation Hello {
    // HelloError
    // HelloError
    errors: [
        // HelloError
    ]
}

As far as I can see, this only happens in oprations and not in services.

milesziemer added a commit to milesziemer/smithy that referenced this issue May 9, 2024
Fixes: smithy-lang#2261

For operation errors like:
```
errors // foo
: // bar
[
    // baz
    MyError
]
```
you'd expect formatting of:
```
// foo
// bar
errors: [
    // baz
    MyError
]
```
Prior to this commit, we were handling the cases of `foo` and `bar`,
but inadvertently doing the same thing for `baz`. This is because
we were looking for comments to pull out to above `errors` in the
direct children of OPERATION_ERRORS, which include all WS within the
`[]`.

To make it work as expected, we need to only pull out comments in the
first two positions (`foo`, `bar`), and leave the rest for BracketFormatter
to format. BracketFormatter was expecting to operate on all the children
of a given TreeCursor, so I modified it to act on an arbitrary stream of
cursors, and added a way to get all remaining siblings after a cursor.
milesziemer added a commit to milesziemer/smithy that referenced this issue May 9, 2024
Fixes: smithy-lang#2261

For operation errors like:
```
errors // foo
: // bar
[
    // baz
    MyError
]
```
you'd expect formatting of:
```
// foo
// bar
errors: [
    // baz
    MyError
]
```
Prior to this commit, we were handling the cases of `foo` and `bar`,
but inadvertently doing the same thing for `baz`. This is because
we were looking for comments to pull out to above `errors` in the
direct children of OPERATION_ERRORS, which include all WS within the
`[]`.

To make it work as expected, we need to only pull out comments in the
first two positions (`foo`, `bar`), and leave the rest for BracketFormatter
to format. BracketFormatter was expecting to operate on all the children
of a given TreeCursor, so I modified it to act on an arbitrary stream of
cursors, and added a way to get all remaining siblings after a cursor.
milesziemer added a commit that referenced this issue May 9, 2024
Fixes: #2261

For operation errors like:
```
errors // foo
: // bar
[
    // baz
    MyError
]
```
you'd expect formatting of:
```
// foo
// bar
errors: [
    // baz
    MyError
]
```
Prior to this commit, we were handling the cases of `foo` and `bar`,
but inadvertently doing the same thing for `baz`. This is because
we were looking for comments to pull out to above `errors` in the
direct children of OPERATION_ERRORS, which include all WS within the
`[]`.

To make it work as expected, we need to only pull out comments in the
first two positions (`foo`, `bar`), and leave the rest for BracketFormatter
to format. BracketFormatter was expecting to operate on all the children
of a given TreeCursor, so I modified it to act on an arbitrary stream of
cursors, and added a way to get all remaining siblings after a cursor.
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 a pull request may close this issue.

1 participant