Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fleet #846

Merged
merged 8 commits into from
Mar 29, 2024
Merged

Fleet #846

merged 8 commits into from
Mar 29, 2024

Conversation

lippfi
Copy link
Contributor

@lippfi lippfi commented Mar 14, 2024

No description provided.

lippfi and others added 8 commits March 3, 2024 22:18
The newer implementation is a part of the vim-engine library and uses new methods from the SearchGroup.kt, but it is not fully refactored yet
The more methods we have in the engine, the fewer number of methods we will need to implement in the Fleet
Pasting was broken with immutable carets because the old caret was not updated during execution
Copy link
Member

@AlexPl292 AlexPl292 left a comment

Choose a reason for hiding this comment

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

A lot of functions in the SearchGroup have no usages in the code. The reason for that is actually stored in the master commit 06e1af3.
The combination of unused code + "revert of revert" significantly slows down the speed of the review as I have to spend more time understanding what changed and the reason behind the change.

In addition, the unused code by itself also leads to the slow down of the review as I have to review this unnecessary code plus understand why there are no usages. And since we have a code quality tool, this will cause extra unnecessary reports from it.

Please handle the comment about the VimPsiService naming in future pull requests.


import com.maddyhome.idea.vim.common.TextRange

public interface VimPsiService {
Copy link
Member

Choose a reason for hiding this comment

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

PSI is an intellij-specific term. As vim-engine is unbound from intellij, this would be better not to use such terms as well.
Inside of IntelliJ IDEA, use of PSI is a more implementation-specific thing. Looking at the methods of this class, it seems that it answers the questions about the language syntax, so probably VimLanguageSyntaxService?

@AlexPl292 AlexPl292 merged commit cb40426 into master Mar 29, 2024
4 checks passed
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