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

Add separate_packages option #2313

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

alex-liang3
Copy link

@alex-liang3 alex-liang3 commented Dec 12, 2024

Resolves #2104 by adding a separate_packages config option. This should also cover #2262.

Sections provided in this field will have the packages within them be separated by a blank line. This also works with custom known_OTHER sections; see tests for examples.

For reference, these changes increase the cyclomatic complexity (PY-R1000) of this function from 68 to 75, so I'm proposing to ignore the check on this function for the same reason as in https://github.com/PyCQA/isort/pull/2340/files#diff-d97e563d18af5b82fdf3bbb340c60da3e6955177a064fcbaf7e8676589933ddaR243

alex-liang3 and others added 2 commits December 12, 2024 11:37
Adds the ability to separate packages within a section with blank lines.
---------

Co-authored-by: Lucas <[email protected]>
chore: use python 3.8-style typing
@vavanade
Copy link

vavanade commented Jan 8, 2025

Any chance of merging this one?

It would really help me. Now I had to write a very ugly hack for the config file to simulate this behavior.

@DanielNoord DanielNoord requested a review from staticdev February 3, 2025 18:56
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.16%. Comparing base (1157b58) to head (7808f3f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2313   +/-   ##
=======================================
  Coverage   99.16%   99.16%           
=======================================
  Files          40       40           
  Lines        3101     3128   +27     
  Branches      680      687    +7     
=======================================
+ Hits         3075     3102   +27     
  Misses         15       15           
  Partials       11       11           

Copy link
Collaborator

@staticdev staticdev left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contributing @alex-liang3. I made some comments with suggestions of how to better benefit from the new option.

@@ -149,6 +153,38 @@ def sorted_imports(
section_output.append("") # Empty line for black compatibility
section_output.append(section_comment_end)

if section in config.separate_packages:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for this @alex-liang3. I know the complexy of this method was already high, but this pirce of code you added here is a perfect candidate for an extract method. With that we could start the needed refactor here to reduce the complexity. Could you please do this refactor?

isort/output.py Outdated
from .settings import DEFAULT_CONFIG, Config


# Ignore DeepSource cyclomatic complexity check for this function. It was
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we dont need to add those lines (see my next comment).

@@ -1039,6 +1039,16 @@ If `True` isort will automatically create section groups by the top-level packag
**Python & Config File Name:** group_by_package
**CLI Flags:** **Not Supported**

## Separate Packages

Separate packages within the listed sections with newlines.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind adding an example here to make it more clear?

* Revert "ci: ignore PY-R1000"

* refactor: breakout logic into separate function

* docs: add examples
@alex-liang3
Copy link
Author

Thanks for the review @staticdev, and all your efforts on this project so far! I've made the requested changes now, but unfortunately the function call itself still adds +1 to the cyclomatic complexity (the criticality threshold is 50).

Let me know what you think of the changes :)

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.

Option to add empty line(s) between top-level package imports
3 participants