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

[925] Improve multiline dictionary and list indentation for sole function parameter #3964

Merged
merged 14 commits into from
Oct 25, 2023

Conversation

henriholopainen
Copy link
Contributor

Description

Making multiline lists and dictionaries less indented when passed as a function parameter and pairing parens with brackets/braces was discussed at in #925. This PR introduces a conservative first step, where a multiline list or dict as the sole parameter to a function call will have special treatment for better readability. The feature is behind hug_parens_with_braces_and_square_brackets preview toggle.

Checklist - did you ...

  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@github-actions
Copy link

github-actions bot commented Oct 22, 2023

diff-shades results comparing this PR (c066e69) to main (ef1048d). The full diff is available in the logs under the "Generate HTML diff report" step.

╭────────────────────────────── Summary ──────────────────────────────╮
│ 19 projects & 1112 files changed / 62 563 changes [+27 944/-34 619] │
│                                                                     │
│ ... out of 2 501 176 lines, 11 760 files & 23 projects              │
╰─────────────────────────────────────────────────────────────────────╯

Differences found.

What is this? | Workflow run | diff-shades documentation

@JelleZijlstra
Copy link
Collaborator

@henriholopainen seems like there's a crash on some code in Hypothesis (from the diff-shades failure). Can you take a look?

@henriholopainen
Copy link
Contributor Author

henriholopainen commented Oct 23, 2023

Located the issue in Hypothesis (and other places); opening and closing brackets/braces don't necessary belong to the same entity. For example:

foooooooooooooooooooo([{c: n + 1 for c in range(256)} for n in range(100)] + [{}], {size})

becomes currently:

foooooooooooooooooooo([
    {c: n + 1 for c in range(256)} for n in range(100)] + [{}], {size
})

which is clearly very wrong. I'll add some test cases and look for a sophisticated approach for a fix tomorrow.

@henriholopainen
Copy link
Contributor Author

Test cases added and logic fixed

CHANGES.md Outdated Show resolved Hide resolved
src/black/linegen.py Show resolved Hide resolved
src/black/linegen.py Show resolved Hide resolved
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@JelleZijlstra JelleZijlstra merged commit 1d4c31a into psf:main Oct 25, 2023
33 of 34 checks passed
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Oct 27, 2023

Thanks for the PR! While I like the style, it's a fairly substantive change for black and goes against the philosophy of diff reduction (in the case you add or remove arguments). I wonder therefore if it makes sense to have magic trailing commas after the single arg prevent this reformatting.

@henriholopainen
Copy link
Contributor Author

Thanks for the PR! While I like the style, it's a fairly substantive change for black and goes against the philosophy of diff reduction (in the case you add or remove arguments). I wonder therefore if it makes sense to have magic trailing commas after the single arg prevent this reformatting.

Do you mean that we should instead format

foo({0: "looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong line"},)

as it would be formatted without the trailing comma:

foo({
    0: "looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong line"
})

?

konstin added a commit to astral-sh/ruff that referenced this pull request Oct 27, 2023
@Avasam
Copy link

Avasam commented Oct 27, 2023

How does (or should) this handle unpacking? I don't see a test to confirm intended behaviour there.

foo(*[
	pretend,
	many,
	and,
	long,
	arguments,
	# ... ..................................................................
	*another_list
])

@henriholopainen
Copy link
Contributor Author

henriholopainen commented Oct 27, 2023

How does (or should) this handle unpacking? I don't see a test to confirm intended behaviour there.

Currently it doesn't, but adding support for that would make sense IMO.

@hauntsaninja
Copy link
Collaborator

It looks like Black already does what I suggested, I went ahead and added a test case and docs for it here: #3991

@JelleZijlstra
Copy link
Collaborator

Maybe I was too hasty in merging this. I think this makes things generally look better, but we should definitely think more about whether we want to go through before we move this change to stable.

@charliermarsh
Copy link
Contributor

One question, as both a user and a Ruff maintainer: is it intended that this also applies to parenthesized lists, sets, etc., outside of function calls? For example, with this commit:

# Input
([1, 2, 3,])

# HEAD^
(
    [
        1,
        2,
        3,
    ]
)

# HEAD
([
    1,
    2,
    3,
])

@JelleZijlstra
Copy link
Collaborator

Not intentionally, but I don't mind that we remove the extra newlines there. Ideally we should also remove the parentheses.

@cthoyt
Copy link

cthoyt commented Nov 8, 2023

This makes me so very happy, thanks @JelleZijlstra :)

charliermarsh added a commit to astral-sh/ruff that referenced this pull request Dec 1, 2023
)

## Summary

This PR implement's Black's new single-argument hugging for lists, sets,
and dictionaries under preview style.

For example, this:

```python
foo(
    [
        1,
        2,
        3,
    ]
)
```

Would instead now be formatted as:

```python
foo([
    1,
    2,
    3,
])
```

A couple notes:

- This doesn't apply when the argument has a magic trailing comma.
- This _does_ apply when the argument is starred or double-starred.
- We don't apply this when there are comments before or after the
argument, though Black does in some cases (and moves the comments
outside the call parentheses).

It doesn't say it in the originating PR
(psf/black#3964), but I think this also applies
to parenthesized expressions? At least, it does in my testing of preview
vs. stable, though it's possible that behavior predated the linked PR.

See: #8279.

## Test Plan

Before:

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

|----------------|------------------:|------------------:|------------------:|
| cpython | 0.75804 | 1799 | 1648 |
| django | 0.99984 | 2772 | 34 |
| home-assistant | 0.99963 | 10596 | 146 |
| poetry | 0.99925 | 317 | 12 |
| transformers | 0.99967 | 2657 | 322 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99980 | 3669 | 18 |
| warehouse | 0.99977 | 654 | 13 |
| zulip | 0.99970 | 1459 | 21 |

After:

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

|----------------|------------------:|------------------:|------------------:|
| cpython | 0.75804 | 1799 | 1648 |
| django | 0.99984 | 2772 | 34 |
| home-assistant | 0.99963 | 10596 | 146 |
| poetry | 0.96215 | 317 | 34 |
| transformers | 0.99967 | 2657 | 322 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99980 | 3669 | 18 |
| warehouse | 0.99977 | 654 | 13 |
| zulip | 0.99970 | 1459 | 21 |
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.

6 participants