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

Rename on-connection-* into tunnel-* and simplify related code. #756

Merged
merged 2 commits into from
Sep 13, 2014

Conversation

vspinu
Copy link
Contributor

@vspinu vspinu commented Aug 29, 2014

The names on-connection-buffer and on-connection-process are pretty confusing. They provide tunneling and are unlikely to be used for anything else, so let's name them as such.

Some simplification/clarification of the code and documentation were made here and there and nrepl-connection-endpoint returns the process instead of the process' buffer-name. I think it's good to work with first principles whenever possible.

I have also increased the process-wait-for-output timout to .05. On some machines, keeping it very low makes Emacs pinging continuously up to the point that C-g doesn't work anymore. This happens mostly on windows but people reported this on linux as well. It doesn't harm to have it higher in this particular place. It's far from noticeable and the process returns faster on any input anyways. Better safe than sorry.

@bbatsov
Copy link
Member

bbatsov commented Sep 3, 2014

See the discussion we had with @hugoduncan regarding those names #489. There're not optimal I guess, but I don't think your suggestion is an improvement.

@vspinu
Copy link
Contributor Author

vspinu commented Sep 3, 2014

The proposed naming is good because nrepl-tunnel-buffer and nrepl-tunnel-process are helpers for nrepl-connection-ssh-tunnel and these names make it very clear.

The naming is not perfect because if there will be some other mechanism which would use a helper process which is not a tunnel, it will require a new variable. But how likely is that? If this ever happens you can rename this vars back to on-connection-xxx or a better name as you will have yet another example of a helper process.

I actually think that the configurable variable nrepl-connection-endpoint is not necessary at this stage. It's simply confusing because nrepl-connection-ssh-tunnel does all the job anyways.

@vspinu vspinu force-pushed the ssh-tunnel branch 4 times, most recently from a732fb4 to 764fddc Compare September 11, 2014 00:00
@vspinu
Copy link
Contributor Author

vspinu commented Sep 11, 2014

I am taking another stab on this. This time I completely removed the customization machinery around ssh tunneling and replaced it with one function nrepl-connection-endpoint which does all the job.

I think this simplicity really wins over the alternative of the unlikely-to-be-used customization and confusing names.

@bbatsov
Copy link
Member

bbatsov commented Sep 12, 2014

@Vitoshka That's fine by me if it's fine by @hugoduncan. I can't think of other realistic use cases for this so the change makes sense to me.

@hugoduncan
Copy link
Member

As long as cider-jack-in in a tramp buffer still works, fine with me.

@bbatsov
Copy link
Member

bbatsov commented Sep 12, 2014

@Vitoshka You'll have to rebase this.

@vspinu
Copy link
Contributor Author

vspinu commented Sep 12, 2014

@Vitoshka That's fine by me if it's fine by @hugoduncan. I can't think of other
realistic use cases for this so the change makes sense to me.

If there are other general use cases then we should code them into
nrepl-connection-endpoint.

If some user would want something very specific then he can always use
advice or completely rewrite nrepl-connection-endpoint for his use. It
just doesn't feel right to have this as a custom variable.

@vspinu
Copy link
Contributor Author

vspinu commented Sep 12, 2014

Rebased on #792 to avoid any further complications.

   - remove configurable nrepl-connection-endpoint
   - nrepl-connection-endpoint is now a function that returns the endpoint (either ssh or plain)
   - nrepl-on-connection-buffer is now nrepl-tunnel-buffer
bbatsov added a commit that referenced this pull request Sep 13, 2014
Rename on-connection-* into tunnel-* and simplify related code.
@bbatsov bbatsov merged commit 017e708 into clojure-emacs:master Sep 13, 2014
@vspinu vspinu deleted the ssh-tunnel branch September 15, 2014 05:16
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.

3 participants