black icon indicating copy to clipboard operation
black copied to clipboard

Fix for #4264: --line-ranges formats entire file when ranges are at EOF

Open sumezulike opened this issue 11 months ago • 1 comments

Description

This fixes #4264. The issue was that the empty last line does not count as a line to adjusted_lines because it is not in the list returned by str.splitlines. Since adjusted_lines is only called on the second pass of _format_str_once in format_str, the first pass would format the code correctly, then the "invalid" line would get removed from lines, and the second pass would format the whole code.

I added a small change to adjusted_lines to cap the end value of any line tuple. For example, --line-ranges 1-100 gets reduced to (1, 4) if the code is only four lines long. One could change if end > original_line_count to if end == original_line_count + 1 to only allow this one additional line, but I think allowing an oversized range to just cover the rest of the code is not surprising behavior, slices act similarly.

I also added a call to adjusted_lines with the unmodified source code before the first pass of _format_str_once. This is an additional computational expense but ensures consistency.

Checklist - did you ...

  • [x] Add an entry in CHANGES.md if necessary?
  • [x] Add / update tests if necessary?
  • [-] Add new / update outdated documentation?

sumezulike avatar Mar 12 '24 16:03 sumezulike

diff-shades reports zero changes comparing this PR (a1ba8778a2c33f051cb66af2468cb8621a1a86c9) to main (1abcffc81816257985678f08c61584ed4287f22a).


What is this? | Workflow run | diff-shades documentation

github-actions[bot] avatar Mar 12 '24 16:03 github-actions[bot]

cc @yilei

JelleZijlstra avatar Mar 13 '24 04:03 JelleZijlstra

Thanks for tagging, @JelleZijlstra!

I also added a call to adjusted_lines with the unmodified source code before the first pass of _format_str_once. This is an additional computational expense but ensures consistency.

Why is this still necessary, after the change to adjusted_lines to cap the end value?

Could you also add a test file next to https://github.com/psf/black/blob/main/tests/data/cases/line_ranges_basic.py ?

This opens up a question on what happens when you specify a --line-ranges= that's outside of the unformatted file. This change makes it valid when you specify a larger <END>, but if the entire range is outside then the entire file is still formatted. How about make it format nothing if everything is outside of the range too? The implementation could be, instead of changing adjusted_lines, to call a new sanitize_lines(lines, src_contents) once in format_str:

def format_str(...):
    if lines:
        lines = sanitize_lines(lines, src_contents)
        if not lines:
            return src_contents  # Nothing to format
    dst_contents = _format_str_once(src_contents, mode=mode, lines=lines)
    ...

yilei avatar Mar 13 '24 19:03 yilei

Thank you so much for the feedback!

Calling adjusted_lines beforehand is not really necessary. I just used it like one would sanitize_lines to make sure that any lines that would be removed on the second pass would already be removed on the first. Writing a new function for that definitely makes more sense!

Thank you also for noticing that moving the entire range out of the file still formats everything, I'll fix that as suggested.

sumezulike avatar Mar 14 '24 08:03 sumezulike

Thanks for reviewing and approving my PR! Glad to be able to contribute to one of my all-time favorite Python projects :)

sumezulike avatar Mar 15 '24 18:03 sumezulike