-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
Treat line numbers as proper marks #180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple minor comments
@@ -5,7 +5,7 @@ command: | |||
partialTargets: | |||
- type: primitive | |||
selectionType: line | |||
modifier: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we add a test case that uses it like a proper mark? Eg "take funk row five"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really have that for the other marks. There is no "take funk that" or similar. But it is treated like the other marks in the code so there really is no difference. If we should add more test for marks in general is a different question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to have that kind of test, but if you think it's better to do those in one pass that's fine too; we don't need to block this PR on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly I think that we probably should do proper tests for all the marks as a separate thing and not just this one specifically.
@@ -5,7 +5,7 @@ command: | |||
partialTargets: | |||
- type: primitive | |||
selectionType: line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this selectionType
comes from talon side? Probably ok but came as a bit of a surprise. I wonder if we could infer that info somewhere in the extension, either during inference or processing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could infer it extension side if we want. But that is true for several other commands as well. This solution has the upside that we don't add additional code we don't actually need to. Instead we reuse the functionality of the line selector and the mark actually only selects the first character of the correct line.
No description provided.