feat: extend the BuildAction to allow setting the architecture #261
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughA new nested enum Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BuildAction
Caller->>BuildAction: Initialize BuildAction(targets, ..., overrideArchitectures)
BuildAction-->>Caller: BuildAction instance with overrideArchitectures property
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Sources/XcodeGraph/Models/BuildAction+Architectures.swift (1)
1-7: Well-designed enum with appropriate protocol conformance.The
Architecturesenum is cleanly implemented with descriptive case names and proper protocol conformance. The extension pattern keeps the code organized.Consider adding documentation comments to improve API discoverability:
extension BuildAction { + /// Architecture configuration options for build actions public enum Architectures: Equatable, Codable, Sendable { + /// Match the architecture of the run destination case matchRunDestination + /// Build for all supported architectures case universal + /// Use the target's architecture settings case useTargetSettings } }Sources/XcodeGraph/Models/BuildAction.swift (1)
38-46: Update DEBUG test helper to include new parameter.The test helper method doesn't include the new
overrideArchitecturesparameter, which could lead to inconsistent behavior between production and test code.extension BuildAction { public static func test( // swiftlint:disable:next force_try targets: [TargetReference] = [TargetReference(projectPath: try! AbsolutePath(validating: "/Project"), name: "App")], preActions: [ExecutionAction] = [], - postActions: [ExecutionAction] = [] + postActions: [ExecutionAction] = [], + overrideArchitectures: Architectures = .useTargetSettings ) -> BuildAction { - BuildAction(targets: targets, preActions: preActions, postActions: postActions) + BuildAction(targets: targets, preActions: preActions, postActions: postActions, overrideArchitectures: overrideArchitectures) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/XcodeGraph/Models/BuildAction+Architectures.swift(1 hunks)Sources/XcodeGraph/Models/BuildAction.swift(2 hunks)Tests/XcodeGraphTests/Models/BuildAction.ArchitecturesTests.swift(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Tests/XcodeGraphTests/Models/BuildAction.ArchitecturesTests.swift (1)
Tests/XcodeGraphTests/Extensions/XCTestCase+Extras.swift (1)
XCTAssertCodable(4-14)
🔇 Additional comments (2)
Sources/XcodeGraph/Models/BuildAction.swift (2)
13-13: LGTM! Property addition follows existing patterns.The new
overrideArchitecturesproperty is correctly implemented and maintains consistency with other properties in the struct.
23-25: LGTM! Initializer update maintains backward compatibility.The initializer correctly includes the new parameter with an appropriate default value that preserves existing behavior.
Also applies to: 32-32
| final class BuildActionArchitecturesTests: XCTestCase { | ||
| func test_codable() { | ||
| // Given | ||
| let subject = BuildAction.Architectures.universal | ||
|
|
||
| // Then | ||
| XCTAssertCodable(subject) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage for all enum cases.
The test correctly verifies Codable conformance but only covers the .universal case. Consider testing all cases to ensure comprehensive coverage.
final class BuildActionArchitecturesTests: XCTestCase {
func test_codable() {
- // Given
- let subject = BuildAction.Architectures.universal
-
- // Then
- XCTAssertCodable(subject)
+ // Test all cases for Codable conformance
+ XCTAssertCodable(BuildAction.Architectures.matchRunDestination)
+ XCTAssertCodable(BuildAction.Architectures.universal)
+ XCTAssertCodable(BuildAction.Architectures.useTargetSettings)
+ }
+
+ func test_equatable() {
+ XCTAssertEqual(BuildAction.Architectures.universal, BuildAction.Architectures.universal)
+ XCTAssertNotEqual(BuildAction.Architectures.universal, BuildAction.Architectures.matchRunDestination)
+ XCTAssertNotEqual(BuildAction.Architectures.universal, BuildAction.Architectures.useTargetSettings)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final class BuildActionArchitecturesTests: XCTestCase { | |
| func test_codable() { | |
| // Given | |
| let subject = BuildAction.Architectures.universal | |
| // Then | |
| XCTAssertCodable(subject) | |
| } | |
| } | |
| final class BuildActionArchitecturesTests: XCTestCase { | |
| func test_codable() { | |
| // Test all cases for Codable conformance | |
| XCTAssertCodable(BuildAction.Architectures.matchRunDestination) | |
| XCTAssertCodable(BuildAction.Architectures.universal) | |
| XCTAssertCodable(BuildAction.Architectures.useTargetSettings) | |
| } | |
| func test_equatable() { | |
| XCTAssertEqual(BuildAction.Architectures.universal, BuildAction.Architectures.universal) | |
| XCTAssertNotEqual(BuildAction.Architectures.universal, BuildAction.Architectures.matchRunDestination) | |
| XCTAssertNotEqual(BuildAction.Architectures.universal, BuildAction.Architectures.useTargetSettings) | |
| } | |
| } |
🤖 Prompt for AI Agents
In Tests/XcodeGraphTests/Models/BuildAction.ArchitecturesTests.swift between
lines 6 and 14, the Codable test only covers the .universal case of the
BuildAction.Architectures enum. To fix this, expand the test to iterate over all
enum cases and verify Codable conformance for each one, ensuring comprehensive
test coverage of all possible values.
|
Hi @fortmarek, I will start to work on the tuist PR then. |
| public enum Architectures: Equatable, Codable, Sendable { | ||
| case matchRunDestination | ||
| case universal | ||
| case useTargetSettings | ||
| } |
There was a problem hiding this comment.
Let's not add types in extensions to keep ease symbol resolution to the compiler.
|
@mbentin-cko you can ignore those. I'd focus on adding the |
Add a parameter in the BuildAction to be able to set the architecture strategy for the scheme.
Summary by CodeRabbit
New Features
Tests