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

Split metadata on parameters independently. #1217

Merged
merged 2 commits into from
May 9, 2023
Merged

Conversation

munificent
Copy link
Member

Also, don't indent parameters with metadata annotations.

Before this change, the formatter indents any parameter that contains metadata annotations that end up splitting:

function(
    @required
        int n,
    @Deprecated('Some long deprecation string.')
        String s) {
  ...
}

It also treats all parameters in a parameter list as a unit when deciding how to split their metadata. If any annotation in any parameter splits, then they all do. That's why there's a split after @required in the above example.

The intent of both of these is so that a parameter with unsplit metadata doesn't get confused for an annotation on the next parameter:

function(
    @first('some string') int n,
    @second('another string forces a split')
    String s) {
  ...
}

Note here that there are two parameters, n, and s, and not just a single parameter s with two annotations.

Unfortunately line splitting all of the parameters as a unit is very bad for performance when there is a large number of parameters (#1212). Also, it's not very helpful. In practice, parameter metadata is rare and most parameters that have any annotations only have one.

And the indentation is just strange looking and inconsistent with how annotations are formatted elsewhere. It also means that parameters with split metadata don't align with parameters that don't have metadata.

This change determines whether each parameter's annotations should split independently from the other parameters' and removes that indentation. The above example becomes:

function(
    @required int n,
    @Deprecated('Some long deprecation string.')
    String s) {
  ...
}

This improves performance on large parameter lists and I think looks better on real-world examples. I ran it on a large corpus (2,112,352 lines in 6,911 files) and I think the impact is small enough to not go through the full change process:

293 insertions + 443 deletions = 736 changes
1 changed line for every 2870.04 lines of code
0.3484 changed lines for every 1,000 lines of code

The full diff is: https://gist.github.com/munificent/1dc7361438934a3587f6149049682e29

Fix #1212.

cc @oprypin

Also, don't indent parameters with metadata annotations.

Before this change, the formatter indents any parameter that contains
metadata annotations that end up splitting:

    function(
        @required
            int n,
        @deprecated('Some long deprecation string.')
            String s) {
      ...
    }

It also treats all parameters in a parameter list as a unit when
deciding how to split their metadata. If any annotation in any parameter
splits, then they all do. That's why there's a split after `@required`
in the above example.

The intent of both of these is so that a parameter with unsplit metadata
doesn't get confused for an annotation on the next parameter:

    function(
        @FIRST('some string') int n,
        @second('another string forces a split')
        String s) {
      ...
    }

Note here that there are two parameters, `n`, and `s`, and not just a
single parameter `s` with two annotations.

Unfortunately line splitting all of the parameters as a unit is very bad
for performance when there is a large number of parameters (#1212).
Also, it's not very helpful. In practice, parameter metadata is rare and
most parameters that have any annotations only have one.

And the indentation is just strange looking and inconsistent with how
annotations are formatted elsewhere. It also means that parameters with
split metadata don't align with parameters that don't have metadata.

This change determines whether each parameter's annotations should split
independently from the other parameters' and removes that indentation.
The above example becomes:

    function(
        @required int n,
        @deprecated('Some long deprecation string.')
        String s) {
      ...
    }

This improves performance on large parameter lists and I think looks
better on real-world examples. I ran it on a large corpus (2,112,352
lines in 6,911 files) and I think the impact is small enough to not go
through the full change process:

293 insertions + 443 deletions = 736 changes
1 changed line for every 2870.04 lines of code
0.3484 changed lines for every 1,000 lines of code

The full diff is: https://gist.github.com/munificent/1dc7361438934a3587f6149049682e29

Fix #1212.
@munificent munificent requested a review from natebosch May 8, 2023 21:31
# Conflicts:
#	CHANGELOG.md
@munificent munificent merged commit 156f5c8 into main May 9, 2023
@munificent munificent deleted the parameter-metadata branch May 9, 2023 20:11
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.

Important performance regression since commit 7fd45d6 on a very big file
2 participants