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

Avoid infinite-propagation of inline comments when force-splitting imports #4074

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Apr 24, 2023

Summary

In #3521, we modified our force-single-line = true behavior to preserve inline comments on imports when splitting multi-member imports into multiple single-member imports. This is useful, since those inline comments are often suppressions that we want to propagate and preserve. For example given:

from foo import (  # noqa: F401
  bar,
  baz,
)

After #3521, we reformatted that (with force-single-line = true) to:

from foo import bar  # noqa: F401
from foo import baz  # noqa: F401

This is all well and good, but #4062 pointed out a case in which this comment propagate caused us to infinitely duplicate those inline comments in certain cases, leading to an exponential blowup.

Given:

from mypackage.subpackage import (  # long comment that seems to be a problem
    a_long_variable_name_that_causes_problems,
    items,
)

After one round of fixing, we'd produce:

from mypackage.subpackage import (  # long comment that seems to be a problem
    a_long_variable_name_that_causes_problems,
)
from mypackage.subpackage import items  # long comment that seems to be a problem

On the next pass, we'd recombine these imports, and notice that from mypackage.subpackage import items is missing the inline comment # long comment that seems to be a problem, which is instead attached to the member import items. When we then force-split these imports, we'd duplicate the comment:

from mypackage.subpackage import (  # long comment that seems to be a problem
    a_long_variable_name_that_causes_problems,
)
from mypackage.subpackage import (  # long comment that seems to be a problem
    items,  # long comment that seems to be a problem
)

On the next pass, we'd find two inline comments, and we'd duplicate them on each import statement.

Anyway, the fix here is to avoid ever combining separate import statements if they're going to eventually be split. Previously, we'd combine those statements during normalization, then split them up at a later pass via a dedicated method. Instead, if we avoid combining imports and handle the splitting during normalization itself, we avoid all of these issues.

Closes #4062.

@charliermarsh charliermarsh added bug Something isn't working isort Related to import sorting labels Apr 24, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     14.2±0.03ms     2.9 MB/sec    1.00     14.2±0.04ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.6±0.00ms     4.7 MB/sec    1.00      3.5±0.01ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    455.0±0.96µs     6.5 MB/sec    1.00    456.8±0.86µs     6.5 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.0±0.02ms     4.2 MB/sec    1.00      6.0±0.02ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.00      7.2±0.01ms     5.7 MB/sec    1.00      7.1±0.02ms     5.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1623.5±3.51µs    10.3 MB/sec    1.00   1614.1±8.19µs    10.3 MB/sec
linter/default-rules/numpy/globals.py      1.01    180.9±0.74µs    16.3 MB/sec    1.00    178.5±0.57µs    16.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.3±0.00ms     7.7 MB/sec    1.00      3.3±0.01ms     7.7 MB/sec
parser/large/dataset.py                    1.00      5.9±0.00ms     6.9 MB/sec    1.09      6.4±0.01ms     6.4 MB/sec
parser/numpy/ctypeslib.py                  1.00   1155.3±0.89µs    14.4 MB/sec    1.07   1234.7±0.64µs    13.5 MB/sec
parser/numpy/globals.py                    1.00    114.8±0.30µs    25.7 MB/sec    1.04    119.9±0.15µs    24.6 MB/sec
parser/pydantic/types.py                   1.00      2.5±0.00ms    10.1 MB/sec    1.08      2.7±0.00ms     9.4 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     21.1±0.52ms  1969.8 KB/sec    1.00     21.2±0.45ms  1967.5 KB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      5.4±0.16ms     3.1 MB/sec    1.00      5.4±0.15ms     3.1 MB/sec
linter/all-rules/numpy/globals.py          1.00   658.7±20.12µs     4.5 MB/sec    1.00   661.3±21.26µs     4.5 MB/sec
linter/all-rules/pydantic/types.py         1.00      9.0±0.26ms     2.8 MB/sec    1.00      9.0±0.21ms     2.8 MB/sec
linter/default-rules/large/dataset.py      1.00     10.7±0.29ms     3.8 MB/sec    1.00     10.7±0.21ms     3.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01      2.4±0.11ms     7.0 MB/sec    1.00      2.3±0.06ms     7.1 MB/sec
linter/default-rules/numpy/globals.py      1.01    265.2±8.68µs    11.1 MB/sec    1.00   262.9±13.14µs    11.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.9±0.12ms     5.3 MB/sec    1.02      4.9±0.15ms     5.2 MB/sec
parser/large/dataset.py                    1.00      8.4±0.13ms     4.8 MB/sec    1.01      8.4±0.14ms     4.8 MB/sec
parser/numpy/ctypeslib.py                  1.01  1608.0±42.15µs    10.4 MB/sec    1.00  1594.5±35.24µs    10.4 MB/sec
parser/numpy/globals.py                    1.00    160.5±5.16µs    18.4 MB/sec    1.00    160.5±8.00µs    18.4 MB/sec
parser/pydantic/types.py                   1.01      3.7±0.09ms     7.0 MB/sec    1.00      3.6±0.07ms     7.0 MB/sec

@charliermarsh charliermarsh merged commit 407af6e into main Apr 24, 2023
13 checks passed
@charliermarsh charliermarsh deleted the charlie/force-single branch April 24, 2023 04:39
renovate bot added a commit to ixm-one/pytest-cmake-presets that referenced this pull request Apr 25, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.262` ->
`^0.0.263` |
[![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/compatibility-slim/0.0.262)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/confidence-slim/0.0.262)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>charliermarsh/ruff</summary>

###
[`v0.0.263`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.263)

[Compare
Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.262...v0.0.263)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

##### Rules

- \[`flake8-bugbear`] Add `pytest.raises(Exception)` support to B017 by
[@&#8203;alanhdu](https://togithub.com/alanhdu) in
[astral-sh/ruff#4052
- \[`flake8-import-conventions`] Implement new rule `ICN003` to ban
`from ... import ...` for selected modules by
[@&#8203;edgarrmondragon](https://togithub.com/edgarrmondragon) in
[astral-sh/ruff#4040
- \[`pylint`] Implement PLE0302 `unexpected-special-method-signature` by
[@&#8203;mccullocht](https://togithub.com/mccullocht) in
[astral-sh/ruff#4075
- \[`pep8-naming`] Ignore `N815` for `TypedDict` fields by
[@&#8203;JonathanPlasse](https://togithub.com/JonathanPlasse) in
[astral-sh/ruff#4066

##### Bug Fixes

- Avoid `PYI015` for valid default value without annotation by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4043
- Avoid infinite-propagation of inline comments when force-splitting
imports by [@&#8203;charliermarsh](https://togithub.com/charliermarsh)
in
[astral-sh/ruff#4074
- Fix SIM222 and SIM223 false positives and auto-fix by
[@&#8203;JonathanPlasse](https://togithub.com/JonathanPlasse) in
[astral-sh/ruff#4063
- Unify positional and keyword arguments when checking for missing
arguments in docstring by
[@&#8203;evanrittenhouse](https://togithub.com/evanrittenhouse) in
[astral-sh/ruff#4067
- Avoid `RUF008` if field annotation is immutable by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4039
- Increment priority should be (branch-local, global) by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4070
- Ignore `ClassVar` annotation for `RUF008`, `RUF009` by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4081
- Support --fix in watch mode by
[@&#8203;evanrittenhouse](https://togithub.com/evanrittenhouse) in
[astral-sh/ruff#4035

#### New Contributors

- [@&#8203;alanhdu](https://togithub.com/alanhdu) made their first
contribution in
[astral-sh/ruff#4052
- [@&#8203;pronoym99](https://togithub.com/pronoym99) made their first
contribution in
[astral-sh/ruff#4055
- [@&#8203;Secrus](https://togithub.com/Secrus) made their first
contribution in
[astral-sh/ruff#4085
- [@&#8203;madkinsz](https://togithub.com/madkinsz) made their first
contribution in
[astral-sh/ruff#4084
- [@&#8203;mccullocht](https://togithub.com/mccullocht) made their first
contribution in
[astral-sh/ruff#4075

**Full Changelog**:
astral-sh/ruff@v0.0.262...v0.0.263

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS41OC4yIiwidXBkYXRlZEluVmVyIjoiMzUuNTguMiJ9-->

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovate bot added a commit to allenporter/flux-local that referenced this pull request Apr 27, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [ruff](https://togithub.com/charliermarsh/ruff) | `==0.0.262` ->
`==0.0.263` |
[![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/compatibility-slim/0.0.262)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/confidence-slim/0.0.262)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>charliermarsh/ruff</summary>

###
[`v0.0.263`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.263)

[Compare
Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.262...v0.0.263)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

##### Rules

- \[`flake8-bugbear`] Add `pytest.raises(Exception)` support to B017 by
[@&#8203;alanhdu](https://togithub.com/alanhdu) in
[astral-sh/ruff#4052
- \[`flake8-import-conventions`] Implement new rule `ICN003` to ban
`from ... import ...` for selected modules by
[@&#8203;edgarrmondragon](https://togithub.com/edgarrmondragon) in
[astral-sh/ruff#4040
- \[`pylint`] Implement PLE0302 `unexpected-special-method-signature` by
[@&#8203;mccullocht](https://togithub.com/mccullocht) in
[astral-sh/ruff#4075
- \[`pep8-naming`] Ignore `N815` for `TypedDict` fields by
[@&#8203;JonathanPlasse](https://togithub.com/JonathanPlasse) in
[astral-sh/ruff#4066

##### Bug Fixes

- Avoid `PYI015` for valid default value without annotation by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4043
- Avoid infinite-propagation of inline comments when force-splitting
imports by [@&#8203;charliermarsh](https://togithub.com/charliermarsh)
in
[astral-sh/ruff#4074
- Fix SIM222 and SIM223 false positives and auto-fix by
[@&#8203;JonathanPlasse](https://togithub.com/JonathanPlasse) in
[astral-sh/ruff#4063
- Unify positional and keyword arguments when checking for missing
arguments in docstring by
[@&#8203;evanrittenhouse](https://togithub.com/evanrittenhouse) in
[astral-sh/ruff#4067
- Avoid `RUF008` if field annotation is immutable by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4039
- Increment priority should be (branch-local, global) by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4070
- Ignore `ClassVar` annotation for `RUF008`, `RUF009` by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4081
- Support --fix in watch mode by
[@&#8203;evanrittenhouse](https://togithub.com/evanrittenhouse) in
[astral-sh/ruff#4035

#### New Contributors

- [@&#8203;alanhdu](https://togithub.com/alanhdu) made their first
contribution in
[astral-sh/ruff#4052
- [@&#8203;pronoym99](https://togithub.com/pronoym99) made their first
contribution in
[astral-sh/ruff#4055
- [@&#8203;Secrus](https://togithub.com/Secrus) made their first
contribution in
[astral-sh/ruff#4085
- [@&#8203;madkinsz](https://togithub.com/madkinsz) made their first
contribution in
[astral-sh/ruff#4084
- [@&#8203;mccullocht](https://togithub.com/mccullocht) made their first
contribution in
[astral-sh/ruff#4075

**Full Changelog**:
astral-sh/ruff@v0.0.262...v0.0.263

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/allenporter/flux-local).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS41OC4yIiwidXBkYXRlZEluVmVyIjoiMzUuNTguMiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovate bot added a commit to allenporter/pyrainbird that referenced this pull request Apr 27, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [ruff](https://togithub.com/charliermarsh/ruff) | `==0.0.262` ->
`==0.0.263` |
[![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/compatibility-slim/0.0.262)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.263/confidence-slim/0.0.262)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>charliermarsh/ruff</summary>

###
[`v0.0.263`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.263)

[Compare
Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.262...v0.0.263)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

##### What's Changed

##### Rules

- \[`flake8-bugbear`] Add `pytest.raises(Exception)` support to B017 by
[@&#8203;alanhdu](https://togithub.com/alanhdu) in
[astral-sh/ruff#4052
- \[`flake8-import-conventions`] Implement new rule `ICN003` to ban
`from ... import ...` for selected modules by
[@&#8203;edgarrmondragon](https://togithub.com/edgarrmondragon) in
[astral-sh/ruff#4040
- \[`pylint`] Implement PLE0302 `unexpected-special-method-signature` by
[@&#8203;mccullocht](https://togithub.com/mccullocht) in
[astral-sh/ruff#4075
- \[`pep8-naming`] Ignore `N815` for `TypedDict` fields by
[@&#8203;JonathanPlasse](https://togithub.com/JonathanPlasse) in
[astral-sh/ruff#4066

##### Bug Fixes

- Avoid `PYI015` for valid default value without annotation by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4043
- Avoid infinite-propagation of inline comments when force-splitting
imports by [@&#8203;charliermarsh](https://togithub.com/charliermarsh)
in
[astral-sh/ruff#4074
- Fix SIM222 and SIM223 false positives and auto-fix by
[@&#8203;JonathanPlasse](https://togithub.com/JonathanPlasse) in
[astral-sh/ruff#4063
- Unify positional and keyword arguments when checking for missing
arguments in docstring by
[@&#8203;evanrittenhouse](https://togithub.com/evanrittenhouse) in
[astral-sh/ruff#4067
- Avoid `RUF008` if field annotation is immutable by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4039
- Increment priority should be (branch-local, global) by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4070
- Ignore `ClassVar` annotation for `RUF008`, `RUF009` by
[@&#8203;dhruvmanila](https://togithub.com/dhruvmanila) in
[astral-sh/ruff#4081
- Support --fix in watch mode by
[@&#8203;evanrittenhouse](https://togithub.com/evanrittenhouse) in
[astral-sh/ruff#4035

##### New Contributors

- [@&#8203;alanhdu](https://togithub.com/alanhdu) made their first
contribution in
[astral-sh/ruff#4052
- [@&#8203;pronoym99](https://togithub.com/pronoym99) made their first
contribution in
[astral-sh/ruff#4055
- [@&#8203;Secrus](https://togithub.com/Secrus) made their first
contribution in
[astral-sh/ruff#4085
- [@&#8203;madkinsz](https://togithub.com/madkinsz) made their first
contribution in
[astral-sh/ruff#4084
- [@&#8203;mccullocht](https://togithub.com/mccullocht) made their first
contribution in
[astral-sh/ruff#4075

**Full Changelog**:
astral-sh/ruff@v0.0.262...v0.0.263

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/allenporter/pyrainbird).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS41OC4yIiwidXBkYXRlZEluVmVyIjoiMzUuNTguMiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working isort Related to import sorting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite loop in import sorting for long variable names with force-single-line (ruff >= 0.0.256)
1 participant