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

Refine warning about incompatible isort settings #8192

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Oct 25, 2023

Summary

This PR refines the warning shown when running ruff format about incompatible isort settings:

  • Only warn if I001 (unsorted imports) is enabled.

  • lines-after-imports: Warn about values other than -1, 1, and 2` because the formatter enforces at least one empty and at most two empty lines after imports.

  • lines-between-types: Warn about values > 1 because the formatter preserves at most one empty line in nested import blocks.

  • split_on_trailing_comma: Only warn when skip-magic-trailing-comma is true to ensure the formatter preserves the trailing commas.

  • Remove force-single-line: I think removing the warnings is safe. The setting seems to mainly be about removing parentheses around alieses:

    from .utils2 import (
      test_directory as test_directory,
    )
    from .utils2 import (
        test_id as test_id,
    )
    
     # isorted
    from .utils import (
      test_directory as teeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee,
    )
    from .utils import test_id as t2
    from .utils2 import test_directory as test_directory
    from .utils2 import test_id as test_id  

    which the formatter leaves unchanged

  • Only warn about force_wrap_aliases when skip-magic-trailing-comma is true. Isort inserts trailing commas that the formatter respects, except when skip-magic-trailing-comma isn't set to true.

Test Plan

Added test plans. Played around with different examples in the playground to identify the isort behavior.

@MichaReiser
Copy link
Member Author

MichaReiser commented Oct 25, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

@MichaReiser MichaReiser added the formatter Related to the formatter label Oct 25, 2023
@MichaReiser
Copy link
Member Author

@charliermarsh you have a better understanding of isort than I. I would appreciate if you could review the compatibility in detail to make sure we get it right this time.

@MichaReiser MichaReiser marked this pull request as ready for review October 25, 2023 02:01
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.

These look correct to me. I also scanned through the full list of options and confirmed the omitted settings seem compatible too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants