Add support for Kotlin installed by Scoop #486
Conversation
|
I changed the method name of the classes & interface I've added. |
There was a problem hiding this comment.
Thanks for the PR, I've left some notes below. I think it's pretty cool to have support for Scoop, but am still a bit unsure if hardcoding paths for all major packaging tools is the way to go. On the other hand, I don't see a lot of great solutions to that if we can't deduce the Kotlin compiler location from the user's PATH...
| * @param name the name of the JAR | ||
| * @return the candidate full paths to the JAR | ||
| */ | ||
| fun candidateAlternativeLibraryLocations(name: String): Array<String> |
There was a problem hiding this comment.
I am not sure if I really understand this abstraction.
While it's nice to have the implementation separated by OS, this is fundamentally pretty specific to a certain kind of (fallback) library resolution, that shouldn't (IMO) live under a name as generic as org.javacs.kt.util.OSContext. I could imagine fitting ShellPathUtils into an interface like this, but that's a different use-case.
I think, in this case specifically I would probably even drop the interface/classes entirely and just inline the when-check over the OS with the corresponding paths into BackupClassPathResolver's findAlternativeLibraryLocation.
Alternatively, if the BackupClassPathResolver is getting too large, we might want to add a SnapClassPathResolver/ScoopClassPathResolver?
There was a problem hiding this comment.
this is fundamentally pretty specific to a certain kind of (fallback) library resolution, that shouldn't (IMO) live under a name as generic as org.javacs.kt.util.OSContext. I could imagine fitting ShellPathUtils into an interface like this, but that's a different use-case.
I thought other methods that depend on the type of OS may be added in the future. I'll have to change their names.
I think, in this case specifically I would probably even drop the interface/classes entirely and just inline the when-check over the OS with the corresponding paths into BackupClassPathResolver's findAlternativeLibraryLocation.
I oppose it if it's called more than once through the entire of the program. We have to choose the appropriate instance according to user's OS. That that wasn't chosen mustn't be loaded.
The USERPROFILE value is used only in Windows, so it must be a state enclosed in the Windows class.
we might want to add a SnapClassPathResolver/ScoopClassPathResolver?
Do they mean singletons (objects) that implement ClassPathResolver?
Fixes #445