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
Add new InfoPlist.extendingDefault case #448
Conversation
Codecov Report
@@ Coverage Diff @@
## master #448 +/- ##
==========================================
+ Coverage 92.6% 92.69% +0.09%
==========================================
Files 347 350 +3
Lines 17815 18077 +262
==========================================
+ Hits 16497 16756 +259
- Misses 1318 1321 +3
Continue to review full report at Codecov.
|
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.
🥋
Great idea!
👍 Looking good so far
868910e
to
3601cbf
Compare
Generated by 🚫 Danger |
This PR is ready for review @tuist/core |
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 API looks great overall - left some minor comments
Tests/TuistGeneratorTests/Generator/InfoPlistContentProviderTests.swift
Outdated
Show resolved
Hide resolved
53dbe89
to
38bb503
Compare
…hen it's a .extendingDefault Info.plist.
38bb503
to
6a85b39
Compare
@ollieatkinson / @kwridan this is ready for another look. |
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.
👍
@@ -0,0 +1 @@ | |||
Derived/ |
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.
Do we want .gitignore
at this level?
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'd say so. That way we can run the test assuming there are no files in that directory.
let project = Project(name: "MainApp", | ||
settings: settings, | ||
targets: [ | ||
Target(name: "App", | ||
platform: .iOS, | ||
product: .app, | ||
bundleId: "io.tuist.App", | ||
infoPlist: "Config/App-Info.plist", | ||
infoPlist: .extendingDefault(with: [:]), |
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.
is it worth adding a custom value?
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.
That's already being tested with unit tests, so I don't think it adds much value.
Since Tuist seeks to remove duplications and hide intricacies from developers' projects and workflows, the content of the
Info.plist
is a good candidate to be managed by Tuist.Solution 📦
Tuist already supports passing the content of the target's Info.plist to the manifest. However, the whole content needs to be included in the dictionary, providing no benefit to the user compared to having the content in an
Info.plist
file.This PR extends the
InfoPlist
model to add a new case,InfoPlist.extendingDefault(with: ["CFBundleShortVersionString": "3.2.1"])
. Tuist provides the base values based on the product type and the platform and extends them using the values provided by the user.Implementation 👩💻👨💻
InfoPlistContentProvider
DerivedFileGenerator
generation logic to get the content using theInfoPlistContentProvider
.