-
Notifications
You must be signed in to change notification settings - Fork 87
[PM-28540] Simplify project files with Xcodegen templates #2153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2153 +/- ##
==========================================
- Coverage 85.52% 84.18% -1.34%
==========================================
Files 1738 1984 +246
Lines 147216 162582 +15366
==========================================
+ Hits 125909 136875 +10966
- Misses 21307 25707 +4400 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
51342b0 to
d553a80
Compare
|
Claude finished @matt-livefront's task in 2m 40s —— View job Code Review CompleteThe review agent has completed its analysis. The XcodeGen template refactoring successfully reduces duplication while maintaining correct target configurations. Overall Assessment: APPROVE The templates are well-structured and the refactoring is clean. All target configurations have been preserved correctly while significantly reducing configuration file size. |
fedemkr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work Matt, love it! 🎉 👏
Just have a few questions.
| MocksTarget: | ||
| sources: | ||
| - path: ${sourcesPath} | ||
| includes: | ||
| - "**/Fixtures/*" | ||
| - "**/Mocks/*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Should the auto generated Sourcery optional file be added here as well?
- path: ${sourcesPath}/Sourcery/Generated/AutoMockable.generated.swift
optional: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue with this is that not all targets using the MocksTarget template integrate Sourcery. AuthenticatorBridgeKitMocks doesn't use Sourcery (yet), which causes the build to fail if that file doesn't exist. I guess we could add Sourcery for that target or keep it as-is for now. Do you have a preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like the bridge to follow the same pattern at some point but currently there are only 2 mocks in there, so applying it now may be a bit overkill.
Maybe we can revisit this if we have more work being done in the bridge. Does that sound good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I like that.

🎟️ Tracking
PM-28540
📔 Objective
When I added Sourcery to BitwardenKit (#2136), I wondered if we could simplify some of the duplication in our project files.
This is an attempt at using Xcodegen's templates to share some common logic between targets.
There's two categories of templates that I've added:
CommonTarget: this is meant for app or framework targets and mainly includes all files other than tests and test helpers (mocks, fixtures, etc).MocksTarget: a target containing mocks and fixtures.TestTarget: a unit testing target which excludes ViewInspector and snapshot tests.SnapshotTestTarget: a test target for snapshot tests.ViewInspectorTestTarget: a test target for ViewInspector tests.SourceryTarget: a template for a target that needs a Sourcery pre-build script.The templates are defined in project-common.yml.
Going forward, adding Sourcery to a target is simplified to adding
SourceryTargetto the list of a target's templates (and adding the generated file to any test targets):Diff of the project files before and after:
The only noticeable change here of the project files is the removal of the
gitignorefiles, which I intentionally excluded. So even though this changes a bunch of stuff, I hope I've minimized any potential issues 🤞.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes