Linting: Only warn when copying the target Info.plist #1203
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1203 +/- ##
==========================================
+ Coverage 76.61% 76.67% +0.05%
==========================================
Files 291 291
Lines 10140 10138 -2
==========================================
+ Hits 7769 7773 +4
+ Misses 2371 2365 -6
Continue to review full report at Codecov.
|
|
I'm not entirely sure why the code coverage has changed (especially in the files I haven't touched). Can somebody help me out? Thanks. |
ollieatkinson
requested review from
a team and
pepicrft
and removed request for
a team
| let reason = "Info.plist at path \($0.pathString) being copied into the target \(target.name) product." | ||
| return LintingIssue(reason: reason, severity: .warning) | ||
| }) | ||
| if let targetInfoPlistPath = target.infoPlist?.path, files.contains(targetInfoPlistPath) { |
There was a problem hiding this comment.
I'm inclined towards removing the check. Here's my why:
- This logic might need to traverse a large list of files. I don't think it'll be a lot if we compare the time that takes with the total generation time, but every bit of time we add aggregates to the other bits.
- This is a check that Xcode runs early in the process, so it's a bit redundant. Also I think that the error that Xcode surfaces is very obvious, so I don't think we are adding much value from our end.
There was a problem hiding this comment.
I'd be still inclined to keep it because you may write some globs which accidentally include your Info.plist file ... without you knowing.
If we are concerned about performance we can use a Set which is O(1) for contains - but I really don't think that it's worth it right now.
There was a problem hiding this comment.
Let's leave it then. If we see this becomes a performance bottleneck, we can either move those attributes to sets, or consider running linters in parallel, considering that they are not doing permutations of the models.
There was a problem hiding this comment.
That's a good point. I didn't know about the Xcode warning since I've never tried to add the Info.plist to the resources. I'm fine with removing it π
Should I just go ahead and do it or is it something that has to be discussed further? @olejnjak mentioned on the issue that he thinks the warning is quite nice.
There was a problem hiding this comment.
Ah, I didn't see your replies. Just forget my comment π
|
Congrats on your first contribution @sgrgrsn! You rock π |
Resolves #1170
Short description π
When adding a framework (eg. Firebase) that requires you to add a .plist file as a resource Tuist will raise an unnecessary warning about copying an Info.plist file.
While it's still useful to get the warning if you by mistake copy the actual Info.plist file for the target to the resources, I still think we should keep the check as a part of the target linting.
Solution π¦
Instead of checking all the copied file paths to contain the phrase "Info.plist" we should instead check if the defined Info.plist path on the target is contained in the copied resources.
Implementation π©βπ»π¨βπ»
lintCopiedFilesinTargetLinterto check if the target's Info.plist path is contained the list of copied files