Feat(VIM-2143) expand environment variables in source command #1222
Conversation
| val home = System.getProperty("user.home") | ||
| if (home != null) { | ||
| return home + path.substring(1) | ||
| expanded = home + substring(1) |
There was a problem hiding this comment.
Hi, TBH I find a floating substrint(1) extremely confusing. I didn't even realize what the receiver of this function.
Please write an explicit receiver like this.substring.
| } | ||
| return path | ||
|
|
||
| val envRe = Regex("\\$[A-Za-z0-9_]+") |
There was a problem hiding this comment.
I feel like this approach skips some cases. For example, the env variable can have brackets ${PATH}, or the dollar can be escaped like \$PATH.
On windows, the variables use a different syntax %PATH%.
Also, I'm not sure if the env variable can start with the number.
This expansion seems to be a non-trivial task. I'd try to search some existing ways in java/kotlin to expand it. Or, maybe there is an existing utility in the IJ platform (you can ask about it in #ij-dev slack channel).
If nothing will work, let's try to write our own expander, but keeping in mind the cases I described. Probably, it would make sense to read the specification on the env variables (if there is any), or ask AI what cases there are.
| return path | ||
|
|
||
| val envRe = Regex("\\$[A-Za-z0-9_]+") | ||
| val env = System.getenv() |
There was a problem hiding this comment.
As I see from the help you referencing, this expansion may be also useful in other places. Can I ask you to move it to the injector, to one of the classes?
Also, since this functionality is not trivial, it'll require some tests.
|
Also, in the commit message please always add a reference to the commit you're working on. |
There was a problem hiding this comment.
Pull Request Overview
Enhance the :source command by adding user-home and environment-variable expansion to file paths.
- Replaces the old
expandUserhelper with a newvimExpandedstring extension - Implements
~and$VARexpansions directly invimExpanded - Updates command invocation to use
argument.trim().vimExpanded()
Comments suppressed due to low confidence (1)
vim-engine/src/main/kotlin/com/maddyhome/idea/vim/vimscript/model/commands/SourceCommand.kt:51
- There are no unit tests covering the new
vimExpandedlogic; add tests for both tilde and environment-variable expansion to ensure correct behavior.
private fun String.vimExpanded(): String {
| if (path.startsWith("~")) { | ||
| private fun String.vimExpanded(): String { | ||
| var expanded = this | ||
| if (startsWith("~")) { |
There was a problem hiding this comment.
The tilde expansion applies to any string beginning with '', including '' case before expansion.username'. This can produce invalid paths; consider restricting to '/' or the exact '
| if (startsWith("~")) { | |
| if (this == "~" || startsWith("~/")) { |
| } | ||
| return path | ||
|
|
||
| val envRe = Regex("\\$[A-Za-z0-9_]+") |
There was a problem hiding this comment.
[nitpick] Compiling the regex on each call can incur overhead; consider moving envRe to a companion object or top-level constant so it's compiled once.
| val envRe = Regex("\\$[A-Za-z0-9_]+") |
|
Yay, copilot now supports Kotlin 🔥 |
zuberol
changed the title
zuberol
changed the title
|
Hi, I’m closing this PR due to inactivity. If you’d like to resume work on it, you’re welcome to reopen it anytime. |
…cription