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 undocumented deviation: comment line wrapping inside lists #7448

Closed
m-richards opened this issue Sep 17, 2023 · 4 comments · Fixed by #7873
Closed

Formatter undocumented deviation: comment line wrapping inside lists #7448

m-richards opened this issue Sep 17, 2023 · 4 comments · Fixed by #7873
Assignees
Labels
accepted Ready for implementation bug Something isn't working formatter Related to the formatter

Comments

@m-richards
Copy link

Hi, thanks for building ruff, it's a great tool, and I've been watching eargerly for auto-formatter support for a while now. Have just tried it out and wanted to report a few formatting deviations - at least as far as I can tell from reading the readme. Apologies if any of these are duplicates, I've had a quick scan of the open issues and I can't seem to see these exact cases.

# before / with black
def test_to_wkb(self):
    wkbs0 = [
        (
            b"\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00"
            b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
        ),  # POINT (0 0)
    ]
# after running `ruff format`
def test_to_wkb(self):
    wkbs0 = [
        (
            b"\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00"
            b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"  # POINT (0 0)
        ),
    ]

I don't know if this is related to the pragma comment rules? The difference is super minor but black's behaviour is preferable as it is the entire binary sequence inside the parentheses that is the binary encoding of a Point.

@MichaReiser
Copy link
Member

This sounds similar to #7269

Although implementing a generic solution could be challenging. But it may be worth special casing it for implicit concatenated strings.

My recommendation would be to make this a leading comment instead. leading comments tend to work better than trailing comments

def test_to_wkb(self):
    wkbs0 = [
		 # POINT (0 0)
        (
            b"\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00"
            b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
        )
    ]

Although Ruff get's this wrong too (for the same reasons):

def test_to_wkb(self):
    wkbs0 = [
        (
			# POINT (0 0)
            b"\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00"
            b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
        )
    ]

@MichaReiser MichaReiser added formatter Related to the formatter needs-decision Awaiting a decision from a maintainer labels Sep 17, 2023
@MichaReiser MichaReiser added this to the Formatter: Beta milestone Sep 17, 2023
@MichaReiser
Copy link
Member

For comparison, Prettier always moves comments out of the parentheses. This would work well in the second example I made, but prevents you from e.g. documenting the first item.

@MichaReiser MichaReiser removed the needs-decision Awaiting a decision from a maintainer label Sep 27, 2023
@charliermarsh
Copy link
Member

For now, our priority is gonna be to fix the suggested alternative:

def test_to_wkb(self):
    wkbs0 = [
        # POINT (0 0)
        (
            b"\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00"
            b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
        ),
        (1)
    ]

Which we currently format as:

def test_to_wkb(self):
    wkbs0 = [
        (
            # POINT (0 0)
            b"\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00"
            b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
        ),
        (1),
    ]

We want this to be part of the Beta.

It'd be nice to fix the trailing comment case here, but we see it as lower-priority. It may fall out of the same solution.

@charliermarsh charliermarsh added bug Something isn't working accepted Ready for implementation labels Sep 27, 2023
@charliermarsh
Copy link
Member

(This probably requires rethinking how we handle parenthesized comments in general.)

@konstin konstin self-assigned this Oct 9, 2023
konstin added a commit that referenced this issue Oct 19, 2023
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

Fixes #7448
Fixes #7892

I've removed automatic dangling comment formatting, we're doing manual
dangling comment formatting everywhere anyway (the
assert-all-comments-formatted ensures this) and dangling comments would
break the formatting there.

## Test Plan

New test file.

---------

Co-authored-by: Micha Reiser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation bug Something isn't working formatter Related to the formatter
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants