Skip to content

Conversation

Aster89
Copy link
Contributor

@Aster89 Aster89 commented Sep 25, 2025

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

  • I have read and understood YCM's CONTRIBUTING document.
  • I have read and understood YCM's CODE_OF_CONDUCT document.
  • I have included tests for the changes in my PR. If not, I have included a
    rationale for why I haven't.
  • I understand my PR may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Why this change is necessary and useful

My understanding, from what reported in #4305 and what I've experienced, is that the existing code was not written with the idea that a ReplaceChunk could be fed with a start that is the line after the last line of the buffer, i.e. when it is == len( vim_buffer ) (I don't think a request can be made with a number higher than that, e.g. "insert from line 9 in a 7-lines file").

This change copes with that case by setting start_line back to the last line index (I've done start_line -= 1, but maybe start_line = len( vim_buffer ) would be better?), and prepending the last line to the replacement_lines as originally compmuted.

The test I've written passes, and the usecase I've described and screencasted in #4305 is fixed.

[Please explain in detail why the changes in this PR are needed.]


This change is Reviewable

Copy link

codecov bot commented Sep 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.31%. Comparing base (131b182) to head (25a6d02).

❗ There is a different number of reports uploaded between BASE (131b182) and HEAD (25a6d02). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (131b182) HEAD (25a6d02)
6 4
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4311       +/-   ##
===========================================
- Coverage   89.88%   75.31%   -14.57%     
===========================================
  Files          37       30        -7     
  Lines        4763     2901     -1862     
===========================================
- Hits         4281     2185     -2096     
- Misses        482      716      +234     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@puremourning
Copy link
Member

Seems similar to a bug i have fixed in my fork, but wasn't completely happy with.

The issue is when the edit is right at the end of the buffer, correct?

I think this is what I tried.

is your version better? thoughts?

@Aster89
Copy link
Contributor Author

Aster89 commented Sep 25, 2025

The issue is when the edit is right at the end of the buffer, correct?

I'd say right past the end, but yeah.

@Aster89
Copy link
Contributor Author

Aster89 commented Sep 25, 2025

I think this is what I tried.

is your version better? thoughts?

I don't like that while. Makes the reader think that vim_buffer.append( '' ) could happen more than once, whereas it can only happen one.

Incidentally, are you against stating preconditions? What's the way of doing it in Python? I'm referring to the fact that start_line == len( vim_buffer ) is the limit condition this change wants to deal with, but start_line > len( vim_buffer ) is a programming error, so it should be caught. Ideally, I'd change my edit to

  if start_line == len( vim_buffer ):
    start_line = len( vim_buffer ) - 1
    replacement_lines = [ ToBytes( vim_buffer[ -1 ] ) ] + replacement_lines

and then put something like

  if start_line > len( vim_buffer ):
    raise SomeError()

right after the definition start_line to make sure we blow up if our understanding, hence precondition, is wrong.

@Aster89
Copy link
Contributor Author

Aster89 commented Sep 26, 2025

@puremourning , are those test failures relevant?

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