Fix for #4264: --line-ranges formats entire file when ranges are at EOF#4273
Fix for #4264: --line-ranges formats entire file when ranges are at EOF#4273JelleZijlstra merged 3 commits intopsf:mainfrom
Conversation
|
cc @yilei |
|
Thanks for tagging, @JelleZijlstra!
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 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)
... |
|
Thank you so much for the feedback! Calling Thank you also for noticing that moving the entire range out of the file still formats everything, I'll fix that as suggested. |
yilei
left a comment
There was a problem hiding this comment.
Thanks! Looks good to me overall, left a few minor comments.
| 2. def func(arg1, | ||
| 3. arg2, arg3): | ||
| 4. pass | ||
| """ |
There was a problem hiding this comment.
Can you also add a case for source not ending with a newline?
| def foo3(parameter_1, parameter_2, parameter_3, parameter_4, parameter_5, parameter_6, parameter_7): pass | ||
| def foo4(parameter_1, parameter_2, parameter_3, parameter_4, parameter_5, parameter_6, parameter_7): pass | ||
|
|
||
| # Adding some unformated code covering a wide range of syntaxes. |
There was a problem hiding this comment.
I would simply remove the lines below, as this test case just need to verify a completely out-of-range input doesn't format
src/black/ranges.py
Outdated
| if not src_contents: | ||
| return [] | ||
| good_lines = [] | ||
| src_line_count = len(src_contents.splitlines()) |
There was a problem hiding this comment.
Not too strong an opinion, it can be more efficient to do a count("\n") but then you also need to add 1 when it doesn't end with a new line.
There was a problem hiding this comment.
Oh, that's really quite a difference
$ python -m timeit -n 10000 -s "f = open('src/black/__init__.py'); src=f.read(); f.close()" "len(src.splitlines())"
10000 loops, best of 5: 171 usec per loop
$ python -m timeit -n 10000 -s "f = open('src/black/__init__.py'); src=f.read(); f.close()" "src.count('\n')"
10000 loops, best of 5: 36.6 usec per loopI resisted the temptation to write src_contents.count("\n") + src_contents[-1] != "\n" 😄
|
Thanks for reviewing and approving my PR! Glad to be able to contribute to one of my all-time favorite Python projects :) |
Description
This fixes #4264. The issue was that the empty last line does not count as a line to
adjusted_linesbecause it is not in the list returned bystr.splitlines. Sinceadjusted_linesis only called on the second pass of_format_str_onceinformat_str, the first pass would format the code correctly, then the "invalid" line would get removed fromlines, and the second pass would format the whole code.I added a small change to
adjusted_linesto cap the end value of any line tuple. For example,--line-ranges 1-100gets reduced to(1, 4)if the code is only four lines long.One could change
if end > original_line_counttoif end == original_line_count + 1to 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_lineswith the unmodified source code before the first pass of_format_str_once. This is an additional computational expense but ensures consistency.Checklist - did you ...
CHANGES.mdif necessary?