feat: Launch selected Godot version straight from the CLI! - #108
feat: Launch selected Godot version straight from the CLI!#108TheDevRatt wants to merge 1 commit into
Conversation
TheDevRatt
changed the title
wlsnmrk
left a comment
wlsnmrk
left a comment
There was a problem hiding this comment.
Thanks for this! I've provided some suggestions and comments.
| } | ||
|
|
||
| [Fact] | ||
| public async Task Launches_Godot_When_Env_Variable_Is_Valid() { |
There was a problem hiding this comment.
Test names in Chickensoft projects are usually just PascalCase, with no underscores.
| } | ||
|
|
||
| [Fact] | ||
| public async Task Fails_When_GODOT_Target_Does_Not_Exist() { |
There was a problem hiding this comment.
Besides the underscores, I'd suggest using EnvVariable or something in place of GODOT -- all-caps isn't preferred, but PascalCase could lead to confusion between Godot itself and the environment variable.
| public async ValueTask ExecuteAsync(IConsole console) { | ||
| var log = ExecutionContext.CreateLog(console); | ||
|
|
||
| string? godotPath = Environment.GetEnvironmentVariable("GODOT"); |
There was a problem hiding this comment.
For this, I would suggest we wait til #101 is merged and then do:
| string? godotPath = Environment.GetEnvironmentVariable("GODOT"); | |
| var godotRepo = ExecutionContext.Godot.GodotRepo; | |
| var godotPath = godotRepo.GodotSymlinkTarget; |
And then update tests to match. (The reason to wait on #101 is that GodotSymlinkTarget is broken on Windows until that's in.)
There was a problem hiding this comment.
For this change, why should we reference the SymLink and not the environment variable directly? It was my understanding after speaking with @jolexxa that we should reference off of the EnvVariable?
There was a problem hiding this comment.
Hmm, okay. Wait for @jolexxa to weigh in, but in terms of my rationale:
- The environment variable should be pointing to the symlink. (If it isn't, the user's system is configured in a way that's incompatible with GodotEnv.) Given that, we may as well use the symlink directly, instead of referring to the environment variable that's referring to the symlink.
- If the user is launching from CLI with GodotEnv, they should expect GodotEnv to behave in a way that's consistent with the rest of its behavior. Unless I've missed something, there are relatively few places in the codebase where GodotEnv actually uses the
GODOTenvironment variable; it sets it and will print it on the user's request, but that's about it. On the other hand, theGodotRepositoryuses the symlink location and target for managing the installations, changing the environment variable, and updating the start menu/desktop shortcuts. So all in all, I think it's more consistent with the rest of GodotEnv's behavior to use the symlink here too.
There was a problem hiding this comment.
@TheDevRatt what Mark said, essentially. The env var is for the user to launch Godot via the symlink and for use in vscode launch configs or any other scripts that need to point to Godot. Since we are managing the symlink, we can just access it directly.
| string? godotPath = Environment.GetEnvironmentVariable("GODOT"); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(godotPath)) { | ||
| log.Err("❌ The GODOT environment variable is not set."); |
There was a problem hiding this comment.
I think (not 100% sure) that @jolexxa has "removing emoji from the output" on her roadmap for GodotEnv, so we may not want to add new ones. Maybe she can confirm if I'm remembering that correctly, though.
| if (SystemInfo.OS == OSType.Windows && !godotPath.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)) { | ||
| godotPath += ".exe"; | ||
| } |
There was a problem hiding this comment.
Here again I'd suggest waiting for #101 - this will also be handled by GodotRepository.GodotSymlinkTarget once that's merged, so we won't need this block at all.
| } | ||
|
|
||
| log.Print($"🚀 Launching Godot from {godotPath}..."); | ||
| await ExecutionContext.Godot.GodotRepo.ProcessRunner.RunDetached(godotPath, []); |
There was a problem hiding this comment.
| await ExecutionContext.Godot.GodotRepo.ProcessRunner.RunDetached(godotPath, []); | |
| var computer = ExecutionContext.Godot.Platform.Computer; | |
| var shell = computer.CreateShell(ExecutionContext.WorkingDir); | |
| shell.RunDetached(godotPath, []); |
This suggestion will require implementing Shell.RunDetached(string executable, string[] args) and adding it to IShell.
|
Converted to draft while I wait on #101. |
|
@TheDevRatt just wanted to let you know that #101 has been merged. |
3 participants
Heya! This is a dual Issue/Feature for GodotEnv. SymLinks aren't working the way they're supposed to on Windows, so I decided (as an avid hater of shortcuts on my beautiful wallpaper) to include a new function inside of the cli that lets you launch your selected Godot version.
I included also, a "detached" mode in the process runner so that after launching the terminal doesn't just sit and wait around due to the application being launched.
This launches the Godot executable based on the environment variable, I've only tested this on windows - but I'm fairly certain Mac and Linux should work as well. Also, I wrote a test to ensure that the command works although it gave me a LOT of headaches and I had to rely pretty heavily on AI for the test specifically, so if something doesn't look right there, that is likely why.
Let me know what you think!