Swift Package fixes #444
Swift Package fixes #444
Conversation
Codecov Report
@@ Coverage Diff @@
## master #444 +/- ##
=======================================
Coverage 80.37% 80.37%
=======================================
Files 149 149
Lines 7635 7635
=======================================
Hits 6137 6137
Misses 1498 1498
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #444 +/- ##
=========================================
+ Coverage 80.37% 80.4% +0.02%
=========================================
Files 149 149
Lines 7635 7635
=========================================
+ Hits 6137 6139 +2
+ Misses 1498 1496 -2
Continue to review full report at Codecov.
|
|
I don't like that Danger requires a changelog. Many things don't require an entry, and some PR's like this aren't new features they are just fixing what is in a previous one that has not been released yet |
|
@pepibumur was there a reason that |
| @@ -125,15 +125,15 @@ public final class PBXProject: PBXObject { | |||
| } | |||
|
|
|||
| /// Package references. | |||
| var packageReferences: [PBXObjectReference]? | |||
There was a problem hiding this comment.
What we've been doing for deciding whether a property should be optional or not is whether Xcode is able to open a project without that attribute. If I'm not wrong, Xcode is able to open a project that doesn't have this attribute. If that's the case, I'd keep this property as optional. It also ensures that we are not adding attributes to Xcode 10 projects that are are not readable by Xcode 10.
There was a problem hiding this comment.
I just changed it to an empty array if there is nothing to decode, and only encode if there's something in it. I'm not sure I understand you, does this mess up a specific use case?
There was a problem hiding this comment.
I'd say this change makes sense - optional arrays are often troubling when it comes to accessing their contents. I can't see a situation where this will cause a problem.
If you want to check the presence of a package inside the Xcodeproj it's possible to by calling packageReferences.count > 0.
There was a problem hiding this comment.
Let's take the following scenario:
- The user edits a Xcode project that doesn't have the
packageReferencesattribute. - We decode the attribute to an empty array.
- After saving the project, the user will see in the diff that a new attribute is added.
Xcode 10.x will ignore the attribute, but we are introducing unexpected modifications to the project. That's something we've always avoided, and I don't think it's solvable by not writing the array when it's empty, because that's another possible value for the attribute.
If I'm not wrong, I think Xcode 11 updates some attribute in the project to say that the project is compatible with version X of Xcode. What about using that value to decide whether the attribute should be written or not?
There was a problem hiding this comment.
@pepibumur What do you mean by attribute? The packageReferences? If the array is empty we don't write it to the project, exactly as if it was nil
There was a problem hiding this comment.
Yes, let me put it on a table:
| Project in disk | Read project | Diff? |
|---|---|---|
| No packageReferences | Empty packageReferences | Yes |
| Empty packageReferences | Empty packageReferences | No |
| Non-empty packageReferences | Non-empty packageReferences | No |
By turning nil into an empty array we'll get a diff after writing the project to disk in the first scenario.
There was a problem hiding this comment.
I assume'd we wouldn't write Empty packageReferences. Is there a benefit to doing so? If we assume empty means we don't write it then we don't have to worry about the truth table above since we only handle two scenarios, empty and non-empty
There was a problem hiding this comment.
There isn't any difference for Xcode. The point is that users don't want to see git diffs if the project hasn't been modified, and with the current implementation they will. I suggest the following modification:
// This one remains optional
var packageReferences: [PBXObjectReference]?
// We define this one as non-optional
public var packages: [XCRemoteSwiftPackageReference] {
set {
packageReferences = newValue.references()
}
get {
return packageReferences.objects() ?? []
}
}Does that look good @ollieatkinson / @yonaskolb ?
There was a problem hiding this comment.
That makes sense - 👍
| @@ -111,17 +111,17 @@ public class XCRemoteSwiftPackageReference: PBXContainerItem, PlistSerializable | |||
| } | |||
|
|
|||
| /// Repository url. | |||
| var repositoryURL: String? | |||
| public var repositoryURL: String? | |||
As far as I'm aware the Danger rule is a warning for the CHANGELOG file - not a requirement for merge. Is that not the case? |
|
That's why Danger is a warning and not erroring. We can't determine whether changes need documentation. The developer opening the PR is responsible for making the call here. |
I used the name that Xcode uses but you are right, it makes more sense to use the name of the property in the project. I'll update it. |
.swiftpmdirectory to.gitignorefor when opening thePackage.swiftin Xcode 11PBXProject.packagesandPBXTarget.packageProductDependencies