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.

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

Formatter instability: trailing clause comments in if statement #7602

Closed
charliermarsh opened this issue Sep 22, 2023 · 2 comments · Fixed by #7608 or #7609
Closed

Formatter instability: trailing clause comments in if statement #7602

charliermarsh opened this issue Sep 22, 2023 · 2 comments · Fixed by #7608 or #7609
Assignees
@charliermarsh
Labels
bug Something isn't working formatter Related to the formatter
Milestone
Formatter: Beta

Comments

@charliermarsh
Copy link
Member

charliermarsh commented Sep 22, 2023 •
edited
Loading

Given:

def Get_x_csrf_token(cookie, showErrors=bool):
    try:
        try:
            socket.create_connection(("google.com", 80))
        except socket.gaierror:
            if showErrors == True:
                raise APIConnectFailed(
                    "No internet connection, check your internet connection and try again."
                )
            else:
                return
            # req.urlopen('http://google.com')
            # print("No internet connection, quiting.")
            # exit(-1)
        # try:
        # except:
        xsrfRequest = requests.post(
            "https://auth.roblox.com/v2/logout", cookies={".ROBLOSECURITY": cookie}
        )
        if xsrfRequest.headers:
            if xsrfRequest.headers.get("x-csrf-token") == None:
                if showErrors == True:
                    raise InvalidCookie(
                        "Invalid .ROBLOSECURITY cookie, are you sure you're using your current cookie ?"
                    )
                else:
                    return
            else:
                return xsrfRequest.headers["x-csrf-token"]
        else:
            if showErrors == True:
                raise NoHeadersError(
                    "The API didn't return any response headers. Check your internet connection."
                )
            else:
                return
                # APIError("The request returned no result headers, Response: ",xsrfRequest.status_code)

            # else:
            # print(xsrfRequest.headers["x-csrf-token"],xsrfRequest.headers)

    except Exception as e:
        raise GetTokenException(
            f"'Get x-csrf-token' process returned an Exception: {e}'"
        )

We're removing the newline after # print(xsrfRequest.headers["x-csrf-token"],xsrfRequest.headers), which leads to an instability.

The above code is the once-formatted version of:

def Get_x_csrf_token(cookie,showErrors=bool):
    try:
        try:
            socket.create_connection(('google.com', 80))
        except socket.gaierror:
            if showErrors == True:
                raise APIConnectFailed("No internet connection, check your internet connection and try again.")
            else:
                return
        #try:
            #req.urlopen('http://google.com')
        #except:
            #print("No internet connection, quiting.")
            #exit(-1)
        xsrfRequest = requests.post("https://auth.roblox.com/v2/logout",
                                    cookies={".ROBLOSECURITY": cookie})
        if xsrfRequest.headers:

            if xsrfRequest.headers.get("x-csrf-token") == None:
                if showErrors == True:
                    raise InvalidCookie("Invalid .ROBLOSECURITY cookie, are you sure you're using your current cookie ?")
                else:
                    return
            else:
                return xsrfRequest.headers["x-csrf-token"]
        else:
            if showErrors == True:
                raise NoHeadersError("The API didn't return any response headers. Check your internet connection.")
            else:
                return


            #else:
                #APIError("The request returned no result headers, Response: ",xsrfRequest.status_code)
            #print(xsrfRequest.headers["x-csrf-token"],xsrfRequest.headers)

    except Exception as e:
        raise GetTokenException(f"'Get x-csrf-token' process returned an Exception: {e}'")

Sourced from #7590 (A_14230110707883433080.py).
@charliermarsh charliermarsh added bug Something isn't working formatter Related to the formatter labels Sep 22, 2023
@charliermarsh charliermarsh added this to the Formatter: Beta milestone Sep 22, 2023
@charliermarsh charliermarsh self-assigned this Sep 22, 2023
@MichaReiser
Copy link
Member

This is fixed on main

@charliermarsh charliermarsh reopened this Sep 22, 2023
@MichaReiser
Copy link
Member

More minimal repro. It's related with b having a larger indent than a and c

https://play.ruff.rs/7723b186-8aa4-4572-8f1e-ec871229be42

This was referenced Sep 22, 2023
Merged
Merged
charliermarsh added a commit that referenced this issue Sep 22, 2023
@charliermarsh
Avoid reordering mixed-indent-level comments after branches (#7609)
1a4f2a9 
## Summary

Given:

```python
if True:
    if True:
        if True:
            pass

        #a
            #b
        #c
else:
    pass
```

When determining the placement of the various comments, we compute the
indentation depth of each comment, and then compare it to the depth of
the previous statement. It turns out this can lead to reordering
comments, e.g., above, `#b` is assigned as a trailing comment of `pass`,
and so gets reordered above `#a`.

This PR modifies the logic such that when we compute the indentation
depth of `#b`, we limit it to at most the indentation depth of `#a`. In
other words, when analyzing comments at the end of branches, we don't
let successive comments go any _deeper_ than their preceding comments.

Closes #7602.

## Test Plan

`cargo test`

No change in similarity.

Before:

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1631 |
| django | 0.99983 | 2760 | 36 |
| transformers | 0.99963 | 2587 | 319 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99979 | 3496 | 22 |
| warehouse | 0.99967 | 648 | 15 |
| zulip | 0.99972 | 1437 | 21 |

After:

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1631 |
| django | 0.99983 | 2760 | 36 |
| transformers | 0.99963 | 2587 | 319 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99979 | 3496 | 22 |
| warehouse | 0.99967 | 648 | 15 |
| zulip | 0.99972 | 1437 | 21 |
charliermarsh added a commit that referenced this issue Sep 25, 2023
@charliermarsh
Preserve newlines after nested compound statements (#7608)
17ceb5d 
## Summary

Given:
```python
if True:
    if True:
        pass
    else:
        pass
        # a

        # b
        # c

else:
    pass
```

We want to preserve the newline after the `# c` (before the `else`).
However, the `last_node` ends at the `pass`, and the comments are
trailing comments on the `pass`, not trailing comments on the
`last_node` (the `if`). As such, when counting the trailing newlines on
the outer `if`, we abort as soon as we see the comment (`# a`).

This PR changes the logic to skip _all_ comments (even those with
newlines between them). This is safe as we know that there are no
"leading" comments on the `else`, so there's no risk of skipping those
accidentally.

Closes #7602.

## Test Plan

No change in compatibility.

Before:

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1631 |
| django | 0.99983 | 2760 | 36 |
| transformers | 0.99963 | 2587 | 319 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99979 | 3496 | 22 |
| warehouse | 0.99967 | 648 | 15 |
| zulip | 0.99972 | 1437 | 21 |

After:

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1631 |
| django | 0.99983 | 2760 | 36 |
| transformers | 0.99963 | 2587 | 319 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99983 | 3496 | 18 |
| warehouse | 0.99967 | 648 | 15 |
| zulip | 0.99972 | 1437 | 21 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@charliermarsh charliermarsh

Labels
bug Something isn't working formatter Related to the formatter
Projects
None yet
Milestone
Formatter: Beta
2 participants
@MichaReiser @charliermarsh