Add support for pre- and post- actions #217
Conversation
|
I've published it at this point to see if there is any early feedback. See the check list for the next steps I will take unless feedback dictates a different direction. |
|
Nicely done @kastiglione. You can go ahead with the remaining steps 👍 |
kastiglione
changed the title
|
I've taken the liberty to add this to the 4.0.0 milestone. I'm going to wrap this up right away so hopefully it's ok that I did that. |
|
This is ready for review. |
There was a problem hiding this comment.
Looks ok to me! It'd be great if either @allu22 or @yonaskolb can give you a second feedback.
|
Thanks @pepibumur, I've added them as reviewers. |
| // MARK: - Build Action | ||
|
|
||
| final public class BuildAction { | ||
| public class SerialAction { |
There was a problem hiding this comment.
Are there any common properties that could go in here in the future? If so we could just call this SchemeAction
There was a problem hiding this comment.
I picked the name for what it is now, since I don't know if it will expand to be more. I figured it could easily be renamed. Having said all that, I can go either way, it's not something I feel strongly about.
There was a problem hiding this comment.
Yeah, it's fine now. Just thought I'd mention it
Sources/xcproj/XCScheme.swift
Outdated
| self.postActions = try element["PostActions"]["ExecutionAction"].all?.map(ExecutionAction.init) ?? [] | ||
| } | ||
|
|
||
| fileprivate func addPreActionsPostActions(_ element: AEXMLElement) { |
There was a problem hiding this comment.
maybe we can call this writeXML?
| @@ -5,6 +5,24 @@ | |||
| <BuildAction | |||
| parallelizeBuildables = "YES" | |||
| buildImplicitDependencies = "YES"> | |||
| <PreActions> | |||
There was a problem hiding this comment.
Have you tested opening this scheme in Xcode and see if it changes the format? We want to keep the work achieved by #216. You may have to add something to the attributesOrder in AEXML+XcodeFormat.swift
There was a problem hiding this comment.
These .xcscheme additions were generated by Xcode itself, not by hand. I added actions through the Xcode UI and these were the results. I'll take a look at #216, thanks.
There was a problem hiding this comment.
I made a small addition to attributesOrder in b8fdd76. In each of the updated xmlElement implementations, the insertion of PreActions and PostActions child elements happens before other children are added, which matches Xcode's xml generation.
Sources/xcproj/XCScheme.swift
Outdated
|
|
||
| public init(scriptText: String, title: String? = nil, environmentBuildable: BuildableReference? = nil) { | ||
| self.scriptText = scriptText | ||
| self.title = title ?? "Run Script" |
There was a problem hiding this comment.
What do you think, if you moved the default value to init arguments? This way it is more clear that title is not optional. At the moment, looking at the init method signature, it looks like title is optional.
|
Review feedback has been addressed. |
|
🎉 |
Short description 📝
This pull request adds support for pre-actions and post-actions to generated schemes.
Solution 📦
First, a new class named
ExecutionActionhas been added to XCScheme.swift. This class represents the.xcschemexml element of the same name.Next, pre- and post- action properties are added applicable Action types. Since this code is common among the Action classes, a
SerialActionsuper class is introduced. All Action classes, exceptProfileAction, now subclassSerialActionto inherit pre- and post- action behavior. Xcode disables pre- and post- actions for Analyze actions, and these changes adhere to that. The Xcode documentation states Analyze should use the pre- and post- actions of the Build action.Implementation 👩💻👨💻
ExecutionActionclasspreActionandpostActionproperties toBuildActionpreActionandpostActionproperties to other Action types (e.g.TestAction)This change is