Conversation
dosubot
bot
added
size:XS
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for structured folders in local packages by updating the local package mapping logic across several components. The key changes include:
- Updating the PBXProjectMapper to include an additional groupPath parameter when mapping local packages.
- Modifying the XCPackageMapper to propagate the new groupPath parameter.
- Adjusting the Package model and its extensions to accommodate the new structured folders design.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Sources/XcodeGraphMapper/Mappers/Project/PBXProjectMapper.swift | Updated mapping of local packages with an added groupPath parameter. |
| Sources/XcodeGraphMapper/Mappers/Packages/XCPackageMapper.swift | Modified return value of local package mapping to include groupPath. |
| Sources/XcodeGraphMapper/Extensions/Package+Extensions.swift | Adjusted switch-case matching to destructure the new parameter while ignoring it. |
| Sources/XcodeGraph/Models/Package.swift | Updated the Package enum to support an optional groupPath. |
Comments suppressed due to low confidence (2)
Sources/XcodeGraphMapper/Extensions/Package+Extensions.swift:9
- [nitpick] Document the decision to disregard the groupPath parameter in the conversion to a path string, so that future maintainers understand this intentional omission.
case let .local(path, _):
Sources/XcodeGraph/Models/Package.swift:6
- Ensure that all consumers of the Package API are updated to correctly handle the new optional groupPath parameter.
case local(path: AbsolutePath, groupPath: String?)
| let localPackages = try pbxProject.localPackages.compactMap { | ||
| try packageMapper.map(package: $0, sourceDirectory: sourceDirectory) | ||
| } + localPackagePaths.map { .local(path: $0) } | ||
| } + localPackagePaths.map { .local(path: $0, groupPath: nil) } |
There was a problem hiding this comment.
All local package mappings use nil for groupPath. If structured folders are intended, consider retrieving and passing the appropriate group path when available to improve maintainability.
| let relativePath = try RelativePath(validating: package.relativePath) | ||
| let path = sourceDirectory.appending(relativePath) | ||
| return .local(path: path) | ||
| return .local(path: path, groupPath: nil) |
There was a problem hiding this comment.
Consider propagating groupPath information if available to support structured folder logic, rather than defaulting to nil.
pepicrft
left a comment
pepicrft
left a comment
There was a problem hiding this comment.
@rishatyakushev thanks for doing this. I'd keep this one open until the counterpart in tuist/tuist is approved :)
pepicrft
left a comment
pepicrft
left a comment
There was a problem hiding this comment.
I left some minor comments.
| let relativePath = try RelativePath(validating: package.relativePath) | ||
| let path = sourceDirectory.appending(relativePath) | ||
| return .local(path: path) | ||
| return .local(path: path, groupPath: nil) |
There was a problem hiding this comment.
Can't we get groupPath from XCLocalSwiftPackageReference?
| let localPackages = try pbxProject.localPackages.compactMap { | ||
| try packageMapper.map(package: $0, sourceDirectory: sourceDirectory) | ||
| } + localPackagePaths.map { .local(path: $0) } | ||
| } + localPackagePaths.map { .local(path: $0, groupPath: nil) } |
There was a problem hiding this comment.
Same here, can't we get groupPath?
2 participants
@pepicrft @fortmarek Hello! 👋
When you have a moment, could you please review this PR? I’m aiming to add support for structured folders in local packages (per the discussion here: https://community.tuist.dev/t/how-to-generate-and-keep-structured-folders-local-packages/245), but I’ve hit a roadblock. I’d be grateful for any feedback or a merge. Thank you! 🙏