#788: Add support for IDE_OPTIONS variable per IDE commandlet - #2247
#788: Add support for IDE_OPTIONS variable per IDE commandlet#2247Ali-Shariati-Najafabadi wants to merge 7 commits into
Conversation
Coverage Report for CI Build 32148921752Coverage increased (+0.03%) to 72.941%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions8 previously-covered lines in 2 files lost coverage.
Coverage Stats💛 - Coveralls |
hohwille
left a comment
There was a problem hiding this comment.
@Ali-Shariati-Najafabadi thanks for your PR. Nice implementation of the story including test coverage. Good job 👍
I only left some small remarks for rework.
| /** {@link VariableDefinition} for additional command-line arguments to start eclipse. */ | ||
| VariableDefinitionString ECLIPSE_OPTIONS = new VariableDefinitionString("ECLIPSE_OPTIONS", null); | ||
|
|
||
| /** {@link VariableDefinition} for additional command-line arguments to start intellij. */ | ||
| VariableDefinitionString INTELLIJ_OPTIONS = new VariableDefinitionString("INTELLIJ_OPTIONS", null); | ||
|
|
||
| /** {@link VariableDefinition} for additional command-line arguments to start android studio. */ | ||
| VariableDefinitionString ANDROID_STUDIO_OPTIONS = new VariableDefinitionString("ANDROID_STUDIO_OPTIONS", null); | ||
|
|
||
| /** {@link VariableDefinition} for additional command-line arguments to start pycharm. */ | ||
| VariableDefinitionString PYCHARM_OPTIONS = new VariableDefinitionString("PYCHARM_OPTIONS", null); | ||
|
|
||
| /** {@link VariableDefinition} for additional command-line arguments to start vscode. */ | ||
| VariableDefinitionString VSCODE_OPTIONS = new VariableDefinitionString("VSCODE_OPTIONS", null); | ||
|
|
There was a problem hiding this comment.
We should only create variables when they are actually referenced.
There are also no variables ECLIPSE_VERSION or ECLIPSE_EDITION for example.
BTW: It seems I missed this in the review for these «TOOL»_VM_ARGS that also do not make any sense.
Please also note that when we add a new IDE it will work out of the box and therefore nobody will think of adding a NEW_IDE_OPTIONS and NEW_IDE_VM_ARGS variable here so only add variables that we actually need here. Also we have spyder as IDE that was not added as variable here (see #2293).
| @@ -1 +1,2 @@ | |||
| M2_REPO=~/.m2/repository | |||
| ECLIPSE_OPTIONS=nosplash | |||
There was a problem hiding this comment.
This is just a mocked test. However, tests should be realistic.
The correct option in eclipse is actually -nosplash so this test is IMHO confusing.
https://equinox.eclipseprojects.io/launcher/starting_eclipse_commandline.html#launcher-arguments
Fun fact: I tested the option with recent eclipse and with ide eclipse -nosplash it was showing the splash screen. However, when just running eclipse -nosplash it was working.
IMHO this reveals another problem with this feature: It seems the order of the arguments are relevant.
Seems IDEasy is somewhat buggy in this regard (already without this PR).
…dlet.java Co-authored-by: Jörg Hohwiller <hohwille@users.noreply.github.com>
This PR fixes #788
Implemented changes:
«IDE»_OPTIONS(e.g.INTELLIJ_OPTIONS,ECLIPSE_OPTIONS,VSCODE_OPTIONS,ANDROID_STUDIO_OPTIONS,PYCHARM_OPTIONS) to pass additional command-line arguments when starting an IDE, analogue to the existing globalIDE_OPTIONSIdeToolCommandlet.runTool(List<String>)so it applies to all IDE commandlets (Eclipse, IntelliJ, Android Studio, PyCharm, VSCode/VSCodium) without per-tool duplicationIdeVariables.javaand documented them invariables.adocTesting instructions
INTELLIJ_OPTIONS=nosplashinconf/ide.propertieside intellijand verify the extra argument is passed to the IDE binarymvn -Dtest=EclipseTest,IntellijTest,VscodeTest testin theclimoduleChecklist for this PR
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)internal