Skip to content

fix: check_code misses lines_before_imports-only changes (#2242)#2563

Open
gaoflow wants to merge 1 commit into
PyCQA:mainfrom
gaoflow:fix/check-code-lines-before-imports-2242
Open

fix: check_code misses lines_before_imports-only changes (#2242)#2563
gaoflow wants to merge 1 commit into
PyCQA:mainfrom
gaoflow:fix/check-code-lines-before-imports-2242

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 25, 2026

Copy link
Copy Markdown

Problem

isort.check_code (and isort.core.process) returns True (no changes needed) when lines_before_imports requires adding or removing blank lines before the import block, but the imports themselves are already correctly sorted.

Reported in #2242.

import isort

code = "from a import b, x\n\nfoo = 'bar'\n"

# isort.code() correctly adds the blank line:
assert isort.code(code, lines_before_imports=1) == "\n" + code  # ✓

# But check_code incorrectly says no change is needed:
assert isort.check_code(code, lines_before_imports=1)  # ✗ should be False

Root cause

Two cooperating issues in isort/core.py:

1. _has_changed() used .strip() for comparison — this stripped leading newlines that output.sorted_imports had prepended (via lines_before_imports), making additions invisible:

# Before: both sides stripped → leading blank line difference hidden
return before.strip() != after.strip()

2. Blank lines consumed before the import block were excluded from the comparison — when lines_before_imports >= 0, blank lines preceding the import block are buffered in lines_before and then discarded (not written to the output stream). The raw_import_section passed to _has_changed never included those consumed lines, so removing them was also never detected as a change.

Fix

  • Change _has_changed to use .rstrip() instead of .strip(), preserving leading whitespace differences while still ignoring insignificant trailing newlines.
  • Capture the buffered pre-import blank lines in above_import_section and prepend them to raw_import_section before the comparison, so removed blank lines are detected.

Tests

Added test_check_code_detects_lines_before_import_changes covering:

  • Adding a blank line (0 present → 1 required)
  • Removing a blank line (1 present → 0 required)
  • Already-correct cases (no false positives)

Note: PR #2541 addresses the same issue with only the .rstrip() fix, which handles the "adding blank lines" case. This PR also fixes the "removing blank lines" case via the above_import_section addition, and includes the requested test.

This pull request was prepared with the assistance of AI, under my direction and review.

isort.core.process returned False (no changes needed) when the only
difference between the input and the correctly-formatted output was the
number of blank lines before the import block, controlled by
lines_before_imports.

Two cooperating issues caused this:

1. _has_changed() used .strip() to compare the before/after import
   sections. This stripped any leading newlines that output.sorted_imports
   had prepended (via lines_before_imports), making additions invisible.
   Fix: use .rstrip() so leading whitespace differences are preserved.

2. When blank lines appear before the first import block and
   lines_before_imports >= 0, those lines are buffered in lines_before
   and then discarded (not written) when an import section is present.
   The raw_import_section passed to _has_changed did not include these
   consumed blank lines, so removing them was never detected as a change.
   Fix: capture the buffered lines in above_import_section and prepend
   them to raw_import_section for the comparison.
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.16%. Comparing base (0740d0c) to head (62df020).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2563   +/-   ##
=======================================
  Coverage   99.16%   99.16%           
=======================================
  Files          41       41           
  Lines        3101     3103    +2     
  Branches      671      671           
=======================================
+ Hits         3075     3077    +2     
  Misses         14       14           
  Partials       12       12           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DanielNoord DanielNoord left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you see #2541? And if so, why should we continue with this approach instead of that one?

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.

2 participants