#95 Cleanup GLSPServerLauncher & add CLI parsing #69
Conversation
- Cleanup GLSPServerLauncher (refactor methods & add support to inject additional modules) - Add a CLIParser helper class and configure default options for configuring -- Server port -- consoleLog flag -- logging directory -- logLevel -- help Fixes eclipse-glsp/glsp/issues/95
Add launch configs for Eclipse Requires: eclipse-glsp/glsp-server/pull/69 Part of: eclipse-glsp/glsp#95
| @@ -12,7 +12,8 @@ Require-Bundle: org.eclipse.glsp.api;bundle-version="0.7.0";visibility:=reexport | |||
| com.google.gson;bundle-version="2.8.2", | |||
| org.eclipse.lsp4j;bundle-version="0.7.0", | |||
| org.eclipse.lsp4j.jsonrpc, | |||
| com.google.guava;bundle-version="21.0.0" | |||
| com.google.guava;bundle-version="21.0.0", | |||
| org.apache.commons.cli;bundle-version="1.4.0";visibility:=reexport | |||
There was a problem hiding this comment.
Are we sure about the re-export here? I just want to raise it to make sure we do not cause any troubles down the road with different versions in peoples applications. But I guess apache commons is quite stable.
There was a problem hiding this comment.
In this case we should be on the save side. The latest release of commons.cli is from 2017 😉. However, I doublechecked and we are also reexporting some dependencies (guice, javax.websocket, ecore etc.). Maybe we should reconsider this.
| } | ||
|
|
||
| public Level parseLogLevel() { | ||
| String levelArg = cmd.getOptionValue("ll"); |
There was a problem hiding this comment.
Either this or the help is incorrect, the parameter in the help shows a single 'l'. Just to avoid that I suggest extracting those options into constants, Maybe even public ones as in the example the user had to set specify 'h' for evaluating help as risky string ;-)
| public static void configureLogger( | ||
| final CLIParser parser) throws ParseException, IOException { | ||
| if (parser.isConsoleLog()) { | ||
| configureConsoleLogger(); |
There was a problem hiding this comment.
For the console logger the log level is not considered. Is that on purpose? For me it is just unexpected, especially since in the example launch config both parameters are specified.
Also it seems that is an either-or situation. Should it be possible to have both a file logger and a console logger at the same time? Cause that might be useful as well. It is the same when the File-Logger is configured, it removes all other appenders it seems. Is this necessary?
There was a problem hiding this comment.
I agree that it sometimes might be useful to have both loggers configured. However, we also need the possibility to disable console logging (if the client-server connection is handled via sys in/out streams).
I suggest the following:
- Console logging is enabled per default
- File logging can be enabled/disabled via option
- Console logging can be enabled/disabled via option
- Print warning if file logging is disabled but an argument for the logDir is present
- LogLevel is considered for all loggers
There was a problem hiding this comment.
Yeah, great suggestion, that sounds good!
| } | ||
|
|
||
| public boolean isConsoleLog() { return optionExists("c"); } | ||
|
|
There was a problem hiding this comment.
Since we have utility methods for almost all parameters I suggest also adding one for 'isHelp' that evaluates the 'h'-option.
| this.processName = processName; | ||
| } | ||
|
|
||
| public boolean optionExists(final String identifier) { |
There was a problem hiding this comment.
Just out of curiosity: Is there a reason you chose 'optionExists' over 'hasOption'? In my opinion 'hasOption' is both shorter and more common.
| printHelp(this.processName, options); | ||
| } | ||
|
|
||
| public static void printHelp(final String processName, final Options options) { |
There was a problem hiding this comment.
Super nit-picky: It seems to me that this method would be better located at the LaunchUtil class as it has no connection to the CLIParser whatsoever besides that it takes options.
| } | ||
|
|
||
| public static void configureLogger( | ||
| final CLIParser parser) throws ParseException, IOException { |
There was a problem hiding this comment.
Unnecessary linebreak.
|
Thanks for the fast review. I simply mostly reused the CLIParser class from https://github.com/eclipse-emfcloud/emfcloud-modelserver. So we should probably incorporate the mentioned improvements there as well. |
- Use constants for CLI options and default values - Extract abstract CLI parser base class - Improve logging configuration
|
I addressed the mentioned issues and refactored the CLIParser class as well. |
* #95 Adapt example to conform to GLSPServerLauncher refactoring Add launch configs for Eclipse Requires: eclipse-glsp/glsp-server/pull/69 Part of: eclipse-glsp/glsp#95
…-glsp#69) * eclipse-glsp#95 Cleanup GLSPServerLauncher & add CLI parsing - Cleanup GLSPServerLauncher (refactor methods & add support to inject additional modules) - Add a CLIParser helper class and configure default options for configuring -- Server port -- consoleLog flag -- logging directory -- logLevel -- help Fixes eclipse-glsp/glsp/issues/95
Fixes eclipse-glsp/glsp/issues/95