#25: implement tool commandlet for intellij #297
Conversation
cli/src/main/java/com/devonfw/tools/ide/tool/intellij/Intellij.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/intellij/Intellij.java
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 9497637949Details
💛 - Coveralls |
hohwille
changed the title
added copy recursively with folder name fixed test workarounds removed extra resolve directory workarounds
…nt-ToolCommandlet-for-Intellij
| @@ -6,3 +6,6 @@ | |||
| *.tar binary | |||
| *.bz2 binary | |||
| *.gz binary | |||
|
|
|||
| # special handling for test files | |||
| cli/src/test/resources/ide-projects/intellij/repository/intellij/intellij/default/windows/bin/studio64.exe text | |||
There was a problem hiding this comment.
is this full path really required? Assuming we do some small refactoring and the path changes nobody will remember to also fix this path here?
| cli/src/test/resources/ide-projects/intellij/repository/intellij/intellij/default/windows/bin/studio64.exe text | |
| studio64.exe text |
| } else if (this.context.getSystemInfo().isLinux()) { | ||
| return toolBinPath.resolve(IDEA_BASH_SCRIPT).toString(); | ||
| } else { | ||
| return getToolPath().resolve("IntelliJ IDEA CE.app").resolve("Contents").resolve("MacOS").resolve(IDEA).toString(); |
There was a problem hiding this comment.
IMHO this is still wrong as we discussed in our meeting.
I already merged the PR #359 and resolved the conflicts into this PR.
If we did everything correct there is no such thing anymore than IntelliJ IDEA CA.app inside the software folder.
Also please be aware the different editions of intelliJ will have a different *.app name so your code would never work with the ultimate edition used by many projects (CE stands for Community Edition).
I hope it was already clarified meanwhile why we need the generic MacOS workaround and avoid such ugly OS-switches here and handling of MacOS apps in individual commandlets.
Ideally like in devonfw-ide after installation we can just call idea on any plattform to launch IntelliJ.
This PR therefore still needs some reworking...
| if (this.context.getSystemInfo().isMac()) { | ||
| setMacOsFilePermissions(getToolPath().resolve("IntelliJ IDEA CE.app").resolve("Contents").resolve("MacOS").resolve(IDEA)); | ||
| } |
|
|
||
| if (Files.exists(binaryFile)) { | ||
| String permissionStr = FileAccessImpl.generatePermissionString(493); |
There was a problem hiding this comment.
Seems I missed to discuss on this API.
IMHO this is very bad design:
- numeric file permissions must always be given in octal notation or nobody can understand what
493actually is. If you write it octal as0755at least some bash/unix experienced user will understand what this. - We should avoid direct usage of
FileAccessImplin commandlets. That is why we haveFileAccessinterface. - Why do we provide a method in FileAccessImpl that converts a number to a String and then we again convert the String to a
SetofPosixFilePermissionsin order to then natively set this viaFiles.setPosixFilePermissions? The idea ofFileAccessis to create a higher level API and abstraction to such lower-level Java NIO operations and make our live easier. So if we believe that using numeric (octal) notation is the most readable way why dont we create a method inFileAccessto set file permissions to a given file with a given numeric value? However, it may be even simpler and more readable to simply define constants for the combinations that we need like this:
/** {@link PosixFilePermission}s for "rwxr-xr-x" or 0755. */
public static Set<PosixFilePermission> RWX_RX_RX = Set.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.GROUP_READ, PosixFilePermission.GROUP_EXECUTE, PosixFilePermission.OTHERS_READ, PosixFilePermission.OTHERS_EXECUTE);
- My real suggestion would rather be to add a method
FileAccess.makeExecutable(Path)that will simply implement whatchmod a+x «path»will do and could also be implemented to work on Windows (see also here).
| } finally { | ||
| return; |
There was a problem hiding this comment.
This does not seem to add any value and is IMHO only causing confusion.
| } finally { | |
| return; |
| @Override | ||
| public void installPlugin(PluginDescriptor plugin) { | ||
|
|
There was a problem hiding this comment.
This is not implemented so this PR does not entirely close #25. If you do something like this on purpose, then create a new issue for IntelliJ plugin support and leave a comment here like TODO https://github.com/devonfw/IDEasy/issues/«id». Otherwise we still end up in the same situation that we have currently that some comandlets seem to be present but are incomplete and if we have no issue about the problem, we might forget about it and currently merging this PR would automatically close #25.
Closes #25