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

End-of-line comments are ignored when breaking long lines in the formatter #6377

Closed
charliermarsh opened this issue Aug 6, 2023 · 3 comments
Closed

End-of-line comments are ignored when breaking long lines in the formatter #6377

charliermarsh opened this issue Aug 6, 2023 · 3 comments
Labels
formatter Related to the formatter
Milestone
Formatter: Alpha

Comments

@charliermarsh
Copy link
Member

For example, we treat this as stable formatting:

def get_recent_deployments(threshold_days: int) -> Set[str]:
    # Returns a list of deployments not older than threshold days
    # including `/root/zulip` directory if it exists.
    recent = set()
    threshold_date = (
        datetime.datetime.now() - datetime.timedelta(days=threshold_days)  # noqa: DTZ005
    )

Whereas we should be formatting it as:

def get_recent_deployments(threshold_days: int) -> Set[str]:
    # Returns a list of deployments not older than threshold days
    # including `/root/zulip` directory if it exists.
    recent = set()
    threshold_date = datetime.datetime.now() - datetime.timedelta(
        days=threshold_days
    )  # noqa: DTZ005

Since the # noqa: DTZ005 causes the accompanying line to exceed the line length.
@charliermarsh charliermarsh added the formatter Related to the formatter label Aug 6, 2023
@charliermarsh
Copy link
Member Author

I haven't looked into this at all, but @MichaReiser has mentioned that this is hard (for line suffixes to be included in the break length).

@charliermarsh charliermarsh mentioned this issue Aug 6, 2023
Merged
@MichaReiser
Copy link
Member

MichaReiser commented Aug 6, 2023 •
edited
Loading

Thanks for the detailed report. I don't know if we want to change this. I think you can argue both ways and it adds a fair amount of complexity. See #5630 for the details

Closing to track this in a single issue

@MichaReiser MichaReiser closed this as not planned Won't fix, can't repro, duplicate, stale Aug 6, 2023
@charliermarsh
Copy link
Member Author

Thanks, missed that this was a dupe.

charliermarsh added a commit that referenced this issue Aug 7, 2023
@charliermarsh
Use parenthesized_with_dangling_comments in arguments formatter (#6376
63692b3 
)

## Summary

Fixes an instability whereby this:

```python
def get_recent_deployments(threshold_days: int) -> Set[str]:
    # Returns a list of deployments not older than threshold days
    # including `/root/zulip` directory if it exists.
    recent = set()
    threshold_date = datetime.datetime.now() - datetime.timedelta(  # noqa: DTZ005
        days=threshold_days
    )
```

Was being formatted as:

```python
def get_recent_deployments(threshold_days: int) -> Set[str]:
    # Returns a list of deployments not older than threshold days
    # including `/root/zulip` directory if it exists.
    recent = set()
    threshold_date = (
        datetime.datetime.now()
        - datetime.timedelta(days=threshold_days)  # noqa: DTZ005
    )
```

Which was in turn being formatted as:

```python
def get_recent_deployments(threshold_days: int) -> Set[str]:
    # Returns a list of deployments not older than threshold days
    # including `/root/zulip` directory if it exists.
    recent = set()
    threshold_date = (
        datetime.datetime.now() - datetime.timedelta(days=threshold_days)  # noqa: DTZ005
    )
```

The second-to-third formattings still differs from Black because we
aren't taking the line suffix into account when splitting
(#6377), but the first
formatting is correct and should be unchanged (i.e., the first-to-second
formattings is incorrect, and fixed here).

## Test Plan

`cargo run --bin ruff_dev -- format-dev --stability-check ../zulip`
durumu pushed a commit to durumu/ruff that referenced this issue Aug 12, 2023
@charliermarsh @durumu
Use parenthesized_with_dangling_comments in arguments formatter (as…
4530789 
…tral-sh#6376)

## Summary

Fixes an instability whereby this:

```python
def get_recent_deployments(threshold_days: int) -> Set[str]:
    # Returns a list of deployments not older than threshold days
    # including `/root/zulip` directory if it exists.
    recent = set()
    threshold_date = datetime.datetime.now() - datetime.timedelta(  # noqa: DTZ005
        days=threshold_days
    )
```

Was being formatted as:

```python
def get_recent_deployments(threshold_days: int) -> Set[str]:
    # Returns a list of deployments not older than threshold days
    # including `/root/zulip` directory if it exists.
    recent = set()
    threshold_date = (
        datetime.datetime.now()
        - datetime.timedelta(days=threshold_days)  # noqa: DTZ005
    )
```

Which was in turn being formatted as:

```python
def get_recent_deployments(threshold_days: int) -> Set[str]:
    # Returns a list of deployments not older than threshold days
    # including `/root/zulip` directory if it exists.
    recent = set()
    threshold_date = (
        datetime.datetime.now() - datetime.timedelta(days=threshold_days)  # noqa: DTZ005
    )
```

The second-to-third formattings still differs from Black because we
aren't taking the line suffix into account when splitting
(astral-sh#6377), but the first
formatting is correct and should be unchanged (i.e., the first-to-second
formattings is incorrect, and fixed here).

## Test Plan

`cargo run --bin ruff_dev -- format-dev --stability-check ../zulip`
@MichaReiser MichaReiser added this to the Formatter: Alpha milestone Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
formatter Related to the formatter
Projects
None yet
Milestone
Formatter: Alpha
Development

No branches or pull requests
2 participants
@MichaReiser @charliermarsh