Added execute script feature, script chosen in the settings panel#83
Added execute script feature, script chosen in the settings panel#83
Conversation
# Conflicts: # Modules/Contracts/Generators/PackageGeneratorContract/Sources/PackageGeneratorContract/PackageGeneratorContract.swift # Modules/Generators/PackageGenerator/Sources/PackageGenerator/PackageGenerator.swift # Modules/Generators/PackageGenerator/Sources/PackageGenerator/Utils/Shell.swift Saving custom script path in project configuration json # Conflicts: # Phoenix.xcodeproj/project.pbxproj Passing rootUrl to contentView, to be able to calculate relative path for script UI tweaks
Nonnus
left a comment
Nonnus
left a comment
There was a problem hiding this comment.
This looks great, awesome job!
IMHO I feel that it might be better to clearly name the Script a Shell Script considering that is the only supported option (unlike eg: swift script) both in code (eg: CustomScriptViewModel -> CustomShellScriptViewModel) and on UI (Custom Script -> Custom Shell Script)
Tawa
left a comment
Tawa
left a comment
There was a problem hiding this comment.
Thanks a lot for implementing this feature, and for opening this PR, sorry it took me so long to take a look at it.
I requested some minor changes, but in general the PR looks good. 👍
| try container.encode(swiftVersion, forKey: .swiftVersion) | ||
| try container.encodeIfPresent(defaultOrganizationIdentifier, forKey: .defaultOrganizationIdentifier) | ||
| try container.encode(platforms, forKey: .platforms) | ||
| try container.encode(customScriptPath, forKey: .customScriptPath) |
There was a problem hiding this comment.
I believe this should be encodeIfPresent
| guard fileManager.fileExists(atPath: url.path) else { return } | ||
|
|
||
| let shell = Shell(verbose: true) | ||
| _ = try shell.executeScript(at: url.absoluteURL.absoluteString) |
There was a problem hiding this comment.
The executeScript function already is annotated with @discardableResult, so you can remove the _ = .
| } | ||
|
|
||
| public func runExternalScriptIfNeeded(at url: URL) throws { | ||
| guard fileManager.fileExists(atPath: url.path) else { return } |
There was a problem hiding this comment.
I'm not sure I like that this function does not notify the user when the script file does not exist. Maybe we should throw an error here and show it in the UI for the user to be aware of the issue? Otherwise things might be broken and the user might not know about it.
There was a problem hiding this comment.
I just saw that the shell.executeScript function already throws an error if it fails to execute the file, so let's remove this guard here and rely on the Shell object to communicate that error.
| let relativePath = relativeComponents.joined(separator: "/") | ||
| return relativePath | ||
| } else { | ||
| print("Script file should be under Modules folder") |
There was a problem hiding this comment.
I feel that this should be communicated differently to the user as well, but it's not a blocker to merge this PR.
| output.split(separator: "\n").forEach { line in | ||
| Task { | ||
| await Console.print(level: 1, .none, String(line)) | ||
| } | ||
| } |
There was a problem hiding this comment.
| output.split(separator: "\n").forEach { line in | |
| Task { | |
| await Console.print(level: 1, .none, String(line)) | |
| } | |
| } | |
| Task { | |
| output.split(separator: "\n").forEach { line in | |
| await Console.print(level: 1, .none, String(line)) | |
| } | |
| } |
Here it's best to create 1 task for all lines instead of 1 task per line.
3 participants
Description
This PR adds a capability to Phoenix to run an external bash script after project generation.
The main changes include:
Shell.swiftwrapper to call a bash scriptCustomScriptViewandCustomScriptViewModelfor the UI changePhoenixDocument.projectConfigurationfieldReferences
Generating.mov
Saving.mov