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

refactor: merge VimMode into the api abstraction #27

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

saidelike
Copy link
Contributor

This is an attempt to refactor the code in order to abstract everything into the VimApi abstraction. This PR only deals with VimMode and merge it into VimApi.

Other parts could be merged later, but this is just the starting point if we agree to go this way. I would rather have this merged before going further.

@fidgetingbits fidgetingbits self-requested a review June 14, 2024 03:47
@fidgetingbits
Copy link
Contributor

I still need to look at this, but two questions:

  1. Did you actually refactor anything from the code that was originally in talon-vim? If not, I won't bother diffing to see what you changed and mostly focus on testing, since I know the code worked anyway.

  2. What did you do to test that I should replicate exactly? Just uninstall pynvim and make sure basic functionality still works? I just want to make sure that the baseline expectations you have for doing this still work on linux, but I already forget what they were.

@saidelike
Copy link
Contributor Author

I still need to look at this, but two questions:

1. Did you actually refactor anything from the code that was originally in talon-vim? If not, I won't bother diffing to see what you changed and mostly focus on testing, since I know the code worked anyway.

This PR is a simple refactor of what there was in talon-vim. The idea is to cleanup the code in order to make it easier to maintain. It should not change its functionality. So testing any command that uses RPC is good indeed.

2. What did you do to test that I should replicate exactly? Just uninstall pynvim and make sure basic functionality still works? I just want to make sure that the baseline expectations you have for doing this still work on linux, but I already forget what they were.

I am not sure to follow why you want to uninstall pynvim. This PR still uses pynvim. It is just code refactoring to make it easier to maintain from now on.

Copy link
Contributor

@fidgetingbits fidgetingbits left a comment

Choose a reason for hiding this comment

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

I think if you add the inheritance and rebase to fix the merge conflicts this is fine. I took it for a spin and it seems to work fine.

I dunno why bought I thought this whole PR was something way more complicated, even after taking a quick glance last time, that was gonna cause a bunch of conflicts with talon-vim but is totally something different.

I'll file an issue, but just something to note is the fact that the class inheritance didn't exist and didn't cause any issues makes me think it's worth doing a big typing pass where everything is strongly typed, and then enforce it with mypy.

except ImportError:
pynvim = None

import time


class VimRPC:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a subclass of VimApiInternal ya?

@@ -11,82 +11,82 @@
class Actions:
def vim_set_normal(auto: bool = True):
"""set normal mode"""
v = VimMode()
v = VimAPI()
v.set_normal_mode(auto=auto)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably causing a merge conflict as we removed auto=auto iirc

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