-
Notifications
You must be signed in to change notification settings - Fork 20
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
Update vim/neovim plugins for hole punching (4/5) #97
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
@@ -72,17 +72,17 @@ def _start_agent(self): | |||
]) | |||
self._agent_stdout_iter = iter(self._agent.stdout.readline, b"") | |||
|
|||
message = None |
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.
You don't need this line I believe - the variable is declared inside an if/else
block - since it always is declared, will be in scope after the block as well.
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.
Makes sense - will remove.
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.
@@ -280,17 +280,13 @@ def _handle_apply_patches(self, message): | |||
def _handle_message(self, message): | |||
self._message_handler(message) | |||
|
|||
def start(self, host_ip=None, host_port=None): | |||
def start(self, session_id=None): |
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.
Since the agent still supports the 'old'/direct way of connecting (I think?), we'll wan't to keep both options open here too. But we can do that later on.
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.
Yeah that's true. I think we should support it as a separate command so we can use it for debugging purposes as well. I'll create an issue.
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.
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.
Should we even bother with exposing the old way? It's just more hassle/things to support.
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.
If anything it's useful for debugging 🤷♂️. Since otherwise the rendezvous server needs to be running. It didn't require much effort to support both ways of connecting on the agent so if anything #99 is a nice-to-have.
859986e
to
97fa939
Compare
5d9e166
to
c661149
Compare
97fa939
to
9918df7
Compare
These are changes for vim and neovim to support hole punching. I only made changes to these editors since it was to test end-to-end locally.
:Tandem
now sends theHostSession
message to the agent. The plugin will then print the session ID when it receives it from the agent.:Tandem <session-id>
now sends theJoinSession
message to the agent.We can potentially also explore a way to add bindings for the plugin to send the
ConnectTo
message as well if we want (since the agent still has the ability to "connect" directly to another agent).