-
Notifications
You must be signed in to change notification settings - Fork 10
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
Improve development experience for LSP and Debug Adapter #200
base: develop
Are you sure you want to change the base?
Conversation
add options for using sockets for the LSP and debug adapter add MagikLanguageClient interface for future use (LSP notifications)
Thank you for this PR @TheCrether. Can you elaborate why this would be needed? I.e., do you use an IDE which does not support communication through stdin/stdout? Also, if this would be applied, the used ports should be configurable. |
import org.apache.commons.cli.Option; | ||
import org.apache.commons.cli.Options; | ||
import org.apache.commons.cli.ParseException; | ||
import org.apache.commons.cli.*; |
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.
Please don't do this. This hides the used imports and is perhaps a sign of too much "complexity" in one file.
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.
I think this happens when I commit in my Java IDE (IntelliJ), I didn't pay it much mind since it's just imports 😅. I can refactor my PRs if you want?
} | ||
|
||
/** | ||
* Get the {@link LanguageClient}. | ||
* | ||
* @return Language client. | ||
*/ | ||
public LanguageClient getLanguageClient() { | ||
public MagikLanguageClient getLanguageClient() { |
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.
Why is this needed? It is not used anywhere.
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.
This was used while I was testing custom notifications sent from the server but I left it here since it could be useful in the future and would be less of a hassle to refactor later on. I can of course remove it though
I misread this, I use this in other parts of my code which I have not created a PR for.
magik-language-server/src/main/java/nl/ramsolutions/sw/magik/languageserver/Main.java
Show resolved
Hide resolved
Hi, I use the communication over sockets when I'm testing changes and debugging the LSP/debug adapter (I start the LSP in Debug mode in IntelliJ and hot reload files, it's faster to rebuild etc.) When I'm not changing anything, I use it over stdin/stdout. I'll make the used port configurable 👍 |
One more thing. Do you actually own the IP to these changes, and not your employer? In The Netherlands, lots of IT-related companies claim ownership of all and any IP written in the employee-contract, on and off the job. Whether this is actually lawful is questionable, although I'm sure this depends on the country. I don't want any hassle regarding IP in case I want to do any commercially with this project later on. |
Using VSCode I can also attach to the LSP/debug adapter. The process is started with the jdwp-agent, see https://github.com/StevenLooman/magik-tools/blob/develop/magik-language-server/client-vscode/client/src/language-client.ts#L46. Isn't this usable? |
Thank you, I didn't know about this. I will try this tomorrow. |
I have no direct clauses regarding ownership of IP in my employment contract, though I was allowed to work on this during work hours. The only clause I could find, is in the collective agreement for the IT branch (Austria thing), regarding patent rights on inventions made while working for the company. So I am not sure on how to handle this since this code isn't really an invention which can be patented (or can it?). All in all, I am not sure how to proceed with my current and future changes. |
So, I've thought about the change and remembered why I chose to add the socket functionality. The main reason being: building a JAR and copying it takes longer than just running the application in the IDE. |
I'm sure it does, as you can skip running the tests. You can skip running the tests from maven by adding the I foresee several potential problems with adding the socket option, such as:
|
I would suggest you ask your employer about this.
There are software patents, so I wouldn't state parts of this project cannot be patented. |
Hi Steven,
this pull requests add options to the LSP and Debug Adapter so that they can run on a socket (port 5007 and 5008 respectively). This makes it possible to start the LSP/Debug Adapter in your IDE of choice and interactively debug it there.
LSP
In order to use a LSP running on a socket in VS Code, you can use the following
serverOptions
when creating a newLanguageClient
Debug Adapter (launch.json)
Using the Debug Adapter through a socket is done by specifying
debugServer
in a launch configuration.