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

Implement isort's default-section setting #10149

Merged
merged 2 commits into from
Mar 1, 2024

Conversation

mmerickel
Copy link
Contributor

@mmerickel mmerickel commented Feb 28, 2024

Summary

This fixes #7868.

Support isort's default-section feature which allows any imports that match sections that are not in section-order to be mapped to a specifically named section.

https://pycqa.github.io/isort/docs/configuration/options.html#default-section

This has a few implications:

  • It is no longer required that all known sections are defined in section-order.
  • This is technically a bw-incompat change because currently if folks define custom groups, and do not define a section-order, the code used to add all known sections to section-order while emitting warnings. However, when this happened, users would be seeing warnings so I do not think it should count as a bw-incompat change.

Test Plan

  • Added a new test.
  • Did not break any existing tests.

Finally, I ran the following config against Pyramid's complex codebase that was previously using isort and this change worked there.

pyramid's previous isort config

https://github.com/Pylons/pyramid/blob/5f7e286b0629b0a5f1225fe51172cba77eb8fda1/pyproject.toml#L22-L37

[tool.isort]
profile = "black"
multi_line_output = 3
src_paths = ["src", "tests"]
skip_glob = ["docs/*"]
include_trailing_comma = true
force_grid_wrap = false
combine_as_imports = true
line_length = 79
force_sort_within_sections = true
no_lines_before = "THIRDPARTY"
sections = "FUTURE,THIRDPARTY,FIRSTPARTY,LOCALFOLDER"
default_section = "THIRDPARTY"
known_first_party = "pyramid"

tested with ruff isort config

[tool.ruff.lint.isort]
case-sensitive = true
combine-as-imports = true
force-sort-within-sections = true
section-order = [
    "future",
    "third-party",
    "first-party",
    "local-folder",
]
default-section = "third-party"
known-first-party = [
    "pyramid",
]

Copy link
Contributor

github-actions bot commented Feb 28, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

warning: Detected debug build without --no-cache.
error: Failed to read examples/How_to_handle_rate_limits.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: trailing comma at line 47 column 4

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to read examples/How_to_handle_rate_limits.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: trailing comma at line 47 column 4

@mmerickel mmerickel changed the title add tool.ruff.isort.default-section add ruff.lint.isort.default-section Feb 28, 2024
@archoversight
Copy link

This will allow me to drop my dependency on isort in projects and just use ruff for it all. 👍

@charliermarsh charliermarsh self-assigned this Feb 29, 2024
@charliermarsh charliermarsh added isort Related to import sorting configuration Related to settings and configuration labels Feb 29, 2024
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good work -- thanks @mmerickel!

I'll admit that I have some hesitancy to support this setting. I think it's possible to come up with a more intuitive and user-friendly configuration scheme for the use-case that motivated the change (e.g., if we want users to be able to merge standard library and third-party imports, perhaps we should focus on how to enable that?). At the same time, I understand why it's helpful to follow isort here.

@charliermarsh charliermarsh enabled auto-merge (squash) March 1, 2024 03:25
@charliermarsh charliermarsh changed the title add ruff.lint.isort.default-section Implement isort's default-section setting Mar 1, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) March 1, 2024 03:25
@charliermarsh charliermarsh merged commit c9931a5 into astral-sh:main Mar 1, 2024
17 checks passed
@mmerickel
Copy link
Contributor Author

Thank you @charliermarsh!

@charliermarsh
Copy link
Member

Thank you! Nice PR.

nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
## Summary

This fixes astral-sh#7868.

Support isort's `default-section` feature which allows any imports that
match sections that are not in `section-order` to be mapped to a
specifically named section.


https://pycqa.github.io/isort/docs/configuration/options.html#default-section

This has a few implications:

- It is no longer required that all known sections are defined in
`section-order`.
- This is technically a bw-incompat change because currently if folks
define custom groups, and do not define a `section-order`, the code used
to add all known sections to `section-order` while emitting warnings.
**However, when this happened, users would be seeing warnings so I do
not think it should count as a bw-incompat change.**

## Test Plan

- Added a new test.
- Did not break any existing tests.

Finally, I ran the following config against Pyramid's complex codebase
that was previously using isort and this change worked there.

### pyramid's previous isort config


https://github.com/Pylons/pyramid/blob/5f7e286b0629b0a5f1225fe51172cba77eb8fda1/pyproject.toml#L22-L37

```toml
[tool.isort]
profile = "black"
multi_line_output = 3
src_paths = ["src", "tests"]
skip_glob = ["docs/*"]
include_trailing_comma = true
force_grid_wrap = false
combine_as_imports = true
line_length = 79
force_sort_within_sections = true
no_lines_before = "THIRDPARTY"
sections = "FUTURE,THIRDPARTY,FIRSTPARTY,LOCALFOLDER"
default_section = "THIRDPARTY"
known_first_party = "pyramid"
```

### tested with ruff isort config

```toml
[tool.ruff.lint.isort]
case-sensitive = true
combine-as-imports = true
force-sort-within-sections = true
section-order = [
    "future",
    "third-party",
    "first-party",
    "local-folder",
]
default-section = "third-party"
known-first-party = [
    "pyramid",
]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration isort Related to import sorting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not require built-in sections to be present in section-order
3 participants