feat: Make Target.metadata mutable and mark Target.prune as deprecated
#275
Conversation
pepicrft
requested review from
cschmatzler and
fortmarek
and removed request for
a team
dosubot
bot
added
the
size:S
WalkthroughUpdated dependency pins in Package.resolved; marked Target.prune as deprecated and made Target.metadata mutable; added customWorkingDirectory and useCustomWorkingDirectory properties and initializer/test parameters to RunAction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
pepicrft
changed the title
Target.metadata mutable and mark Target.prune as deprecated
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/XcodeGraph/Models/Target.swift (1)
59-63: Clarify deprecation message wording and include a concrete migration hintThe current message says “we removed it,” while the property still exists (deprecated). It may confuse adopters. Consider rephrasing and giving a concrete tag example for migration.
Apply this diff to tighten wording:
- @available(*, deprecated, message: """ - The prune attribute coupled XcodeGraph to a particular use-case of Tuist and therefore - we removed it in favor of using metadata as an in-memory context holder that can be leveraged - using conventional tags. - """) + @available(*, deprecated, message: """ + The `prune` attribute coupled XcodeGraph to a Tuist-specific use case and is deprecated. + Prefer encoding this intent in `metadata` using conventional tags (e.g., "tuist:prunable"). + """)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Package.resolved(3 hunks)Sources/XcodeGraph/Models/Target.swift(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.swift
📄 CodeRabbit Inference Engine (AGENT.md)
Use Swift 6.0.3+ with StrictConcurrency enabled
Files:
Sources/XcodeGraph/Models/Target.swift
Sources/XcodeGraph/Models/**/*
📄 CodeRabbit Inference Engine (AGENT.md)
Core data structures like Project, Target, Scheme, and BuildConfiguration should be implemented in
Sources/XcodeGraph/Models/
Files:
Sources/XcodeGraph/Models/Target.swift
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SPM Test (ubuntu-22.04, 6.0.3)
- GitHub Check: SPM Build (ubuntu-22.04, 6.0.3)
- GitHub Check: Build
🔇 Additional comments (3)
Package.resolved (3)
2-2: Confirm lockfile change is intentionaloriginHash changed. If this PR’s scope is limited to model changes, consider isolating dependency resolution updates into a separate commit to simplify rollbacks and bisects.
Would you confirm that these lockfile updates are intentional and tied to the Target changes rather than incidental local resolution differences?
36-37: filesystem pin bumped to 0.11.8Looks fine. Please ensure there are no transitive API changes affecting XcodeGraph code paths.
If helpful, I can run a repo-wide grep to spot usages that may have changed between 0.11.5 and 0.11.8.
171-172: xcodeproj pin bumped to 9.5.0This is a relatively central dependency. Confirm no breaking changes impact consumers and that CI passes across supported Swift toolchains.
I can also prepare a small script to scan for symbols removed/renamed in new XcodeProj APIs if you’d like.
dosubot
bot
added
size:M
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Sources/XcodeGraph/Models/RunAction.swift (2)
85-87: Test factory: derive default foruseCustomWorkingDirectoryfromcustomWorkingDirectorySmall ergonomics improvement: when a path is provided, default the flag to true to reduce footguns in tests.
Apply this diff to the test helper signature:
- useCustomWorkingDirectory: Bool = false + useCustomWorkingDirectory: Bool? = nilThen forward a derived value (see suggested change at Lines 103–105).
103-105: Forward derived default foruseCustomWorkingDirectoryin test factoryIf the flag isn’t specified, infer it from the presence of a path.
Apply this diff:
- customWorkingDirectory: customWorkingDirectory, - useCustomWorkingDirectory: useCustomWorkingDirectory + customWorkingDirectory: customWorkingDirectory, + useCustomWorkingDirectory: useCustomWorkingDirectory ?? (customWorkingDirectory != nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/XcodeGraph/Models/RunAction.swift(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.swift
📄 CodeRabbit Inference Engine (AGENT.md)
Use Swift 6.0.3+ with StrictConcurrency enabled
Files:
Sources/XcodeGraph/Models/RunAction.swift
Sources/XcodeGraph/Models/**/*
📄 CodeRabbit Inference Engine (AGENT.md)
Core data structures like Project, Target, Scheme, and BuildConfiguration should be implemented in
Sources/XcodeGraph/Models/
Files:
Sources/XcodeGraph/Models/RunAction.swift
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: SPM Build (macos-15, 6.0.3)
- GitHub Check: SPM Test (macos-15, 6.0.3)
- GitHub Check: SPM Test (ubuntu-22.04, 6.0.3)
- GitHub Check: SPM Build (ubuntu-22.04, 6.0.3)
- GitHub Check: Build
🔇 Additional comments (1)
Sources/XcodeGraph/Models/RunAction.swift (1)
58-59: LGTM: new properties are correctly assignedThe initializer assigns the new fields as expected.
| public var customWorkingDirectory: AbsolutePath? | ||
| public var useCustomWorkingDirectory: Bool |
There was a problem hiding this comment.
Default useCustomWorkingDirectory to avoid breaking Decodable; consider restricting external mutation
Adding a non-optional Bool without a default breaks backward Decoding when the key is absent. Also consider limiting write access to maintain invariants.
Apply this diff to make Decoding backward-compatible and reduce external mutation risks:
- public var customWorkingDirectory: AbsolutePath?
- public var useCustomWorkingDirectory: Bool
+ public internal(set) var customWorkingDirectory: AbsolutePath?
+ public internal(set) var useCustomWorkingDirectory: Bool = false📝 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.
| public var customWorkingDirectory: AbsolutePath? | |
| public var useCustomWorkingDirectory: Bool | |
| public internal(set) var customWorkingDirectory: AbsolutePath? | |
| public internal(set) var useCustomWorkingDirectory: Bool = false |
🤖 Prompt for AI Agents
In Sources/XcodeGraph/Models/RunAction.swift around lines 21–22, make
useCustomWorkingDirectory backward-compatible and less writable by giving it a
default value and restricting external mutation: change its declaration to
provide a default (e.g., = false) so Decodable won’t break when the key is
missing, and limit external write access (e.g., public private(set) var) so
callers cannot mutate it freely; also adjust any explicit initializers or
CodingKeys/Decodable init to respect the default if present.
| appClipInvocationURL: URL? = nil, | ||
| customWorkingDirectory: AbsolutePath? = nil, | ||
| useCustomWorkingDirectory: Bool = false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against invalid state when enabling custom working directory without a path
Ensure we don’t end up with useCustomWorkingDirectory == true and customWorkingDirectory == nil.
Add a precondition at the beginning of the initializer body:
precondition(
!useCustomWorkingDirectory || customWorkingDirectory != nil,
"useCustomWorkingDirectory requires customWorkingDirectory to be non-nil"
)🤖 Prompt for AI Agents
In Sources/XcodeGraph/Models/RunAction.swift around lines 40 to 42, guard
against the invalid state where useCustomWorkingDirectory is true while
customWorkingDirectory is nil by adding a precondition at the start of the
initializer body: check that !useCustomWorkingDirectory ||
customWorkingDirectory != nil and provide a clear message like
"useCustomWorkingDirectory requires customWorkingDirectory to be non-nil".
While working on a Tuist feature that allows keeping the sources of the targets that have been replaced by binaries, I realized the
Target.pruneattribute was tied to a Tuist's specific use case.I believe XcodeGraph should be generic, like middlewares like
Plugare to web servers, and that we should designXcodeGraphmodels to be as generic as possible. Therefore, I'm marking it as deprecated. Instead, I'm going to useTarget.metadata, which can play the role of holding context in memory, in the same way conn assigns do in Elixir. For example, we can use the tagtuist:prunable, to mark a target to be prunable in a way that doesn't couple the design ofXcodeGraph.Targetto a specific use case.Summary by CodeRabbit
New Features
Refactor
Chores