Implement ; and ,

[aminroosta]

Related Issue #410

@rebornix @johnfn please let me know if you want me to change something in this PR.

[johnfn]

First of all, thank youuu for implementing this! This is a big one that a lot of people want, and I'm super excited to get it in VSCodeVim!

I have some suggestions for improvement. Let me know what you think.

1. Do not have a BaseRepeatableMovement subclass.
2. In order to determine the reverse, simply make a mapping from motions to reverse motions. It might look something like this:

const reverseMotionMapping = {
    MoveFindForward: MoveFindBackward,
    MoveFindBackward: MoveFindForward,

    /* etc... */ 
}

Now you don't have to duplicate the keypress assignment and all the information about reversing is clustered together.

[aminroosta]

@johnfn As always great feedback 👍
PR is now updated accordingly.

[johnfn]

Last suggestion: instead of VimState.lastRepeatableMovement = this inside the body of the exec statement, my suggestion would be to have a property on BaseMotion called something like repeatableWithSemicolonOrComma (feel free to come up with a better name if you wish) and set that to true on the related motions (tTfF).

Then after we execute movements, check if that flag is set on the movement we just ran and if so then run VimState.lastRepeatableMovement = this.

Kind of hard to explain exactly why I think this is better but I like a more declarative approach of specifying important characteristics of actions as properties on the class rather than in the body of the function (where it's harder to find). I think it improves readability.

[aminroosta]

@johnfn No wonder that this project has a readable and clean codebase ;)
PR updated accordingly.

[johnfn]

:)

Can you actually make it a property on the class directly, sort of like right under

class MyActionThing extends BaseMovement {
   keys = ["f", "<character>"];

you have

repeatableWithSemicolonOrComma = true

?

[aminroosta]

@johnfn repeatableWIthSemicolonOrComma should only be set to true if the action is successfully performed and there is no operator like "dfa"

[aminroosta]

@johnfn Based on our discussion in slack channel PR updated.

[johnfn]

Last thing, I promise! (Sorry for the delay!)

Can you condense it down to one line in the 4 cases where it appears?

return !vimState.recordedState.operator || !(isIMovement(result) && result.failed);

Then we should legitimately be good to go here.

[aminroosta]

@johnfn Done 😄

[johnfn]

I'm ready to merge this, I just need it to be up to date with master.

[aminroosta]

@johnfn Done