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

Feature: Add support for named-pipe sockets for LSPs #3509

Merged
merged 2 commits into from
Jan 26, 2021

Conversation

Svetlitski
Copy link
Contributor

@Svetlitski Svetlitski commented Jan 1, 2021

Overview:

Previously, ALE assumed that for all linters with lsp == 'socket' that the socket referred to by address was a TCP socket. Neovim's sockconnect also supports named pipes [1] which have lower overhead than TCP sockets. This pull-request thus adds support for using sockets which are named pipes. The very simple heuristic of checking if address contains a colon or not is used to determine if the given socket is TCP or a named pipe, in order to maintain backwards compatibility, and to avoid adding an additional configuration option to ale#linter#Define.

Tests:
A new test case has been added to test/test_socket_connections.vader in order to test named pipe sockets. To support this test, I wrote a very simple named-pipe-based server test/dumb_named_pipe_server.py (which is largely copied from test/dumb_tcp_server.py)

Documentation:

The documentation has been updated with information about this new feature.

[1] What I'm calling "named pipes" here are not actually named pipes (i.e. the things you'd get from calling `mkfifo`), but unix domain sockets. For whatever reason Neovim's documentation conflates these things (see `:help sockconnect`), so I've just tried to be consistent with their terminology

@kevinclark
Copy link
Contributor

kevinclark commented Jan 2, 2021

LGTM! 👍

@hsanson
Copy link
Contributor

hsanson commented Jan 22, 2021

ALE migrated from Travis to Github Actions (see #3548) so it is necessary to rebase this PR from latest master branch to trigger the required checks.

# Add dense-analysis as remote upstream on your local fork. Not 
# needed if already added.
git remote add upstream https://github.com/dense-analysis/ale.git

# Sync your local master with upstream master
git checkout master
git fetch upstream master
git merge upstream/master

# Rebase the PR branch with master
git checkout my-branch-name
git rebase master   # Fix any conflicts you may have

# Force push the updated PR branch to origin to trigger the checks
git push -f origin my-branch-name

@Svetlitski
Copy link
Contributor Author

Closed by accident, whoops. @hsanson, done, looks like everything is passing.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

I'm happy with this. Pretty good addition.

@w0rp w0rp merged commit cab4280 into dense-analysis:master Jan 26, 2021
@w0rp
Copy link
Member

w0rp commented Jan 26, 2021

Cheers! 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants