Skip to content

Conversation

@claude
Copy link

@claude claude bot commented Jan 20, 2026

Area Inspected

Randomly selected and inspected: vim-engine/src/main/kotlin/com/maddyhome/idea/vim/vimscript/model/expressions/RegisterExpression.kt

Issues Found

While reviewing RegisterExpression.kt and its test coverage, I discovered a significant gap in test coverage:

Missing Test Coverage:

  • No tests for E354 error when attempting to assign to readonly registers
  • The assign() method in RegisterExpression.kt correctly throws E354 for readonly registers (%, :, ., #, =), but this behavior was not tested
  • Without these tests, regressions in this error handling could go undetected

Changes Made

Added comprehensive test coverage in LetCommandRegisterLValueTest.kt:

  1. Tests for simple assignment to readonly registers:

    • @% (current filename register)
    • @: (last command register)
    • @. (last inserted text register)
    • @# (alternate buffer register)
    • @= (expression buffer register)
  2. Tests for compound assignment to readonly registers:

    • @%.= (current filename register with string concatenation)
    • @:.= (last command register with string concatenation)

All tests verify that the appropriate E354 error is thrown with the correct error message.

Why This Improves the Code

  1. Prevents Regressions: Ensures that readonly register protection cannot be accidentally broken in future refactorings
  2. Documents Behavior: Tests serve as executable documentation of the expected error handling
  3. Completeness: Brings test coverage in line with the implementation's error handling capabilities
  4. Vim Compatibility: Ensures IdeaVim maintains compatibility with Vim's readonly register behavior

Code Review Notes

The implementation in RegisterExpression.kt is sound:

  • Code style follows Kotlin conventions ✓
  • Null safety is properly handled ✓
  • Error handling is appropriate ✓
  • The logic is clear and maintainable ✓

The only issue was the missing test coverage, which has now been addressed.

🤖 Generated with Claude Code

Added comprehensive test coverage for E354 error when attempting to assign
to readonly registers (%, :, ., #, =). Previously, while RegisterExpression
correctly throws E354 for readonly registers, there were no tests verifying
this behavior.

Tests added:
- Assignment to each readonly register (%, :, ., #, =)
- Compound assignment operators to readonly registers

This ensures the error handling in RegisterExpression.assign() is properly
tested and prevents regressions.

Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
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.

0 participants