-
-
Notifications
You must be signed in to change notification settings - Fork 303
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
Review optionality of attributes #107
Conversation
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.
LGTM!!
Amazing job as always Pedro
@@ -14,7 +14,7 @@ public class PBXBuildPhase: PBXObject { | |||
|
|||
public init(reference: String, | |||
files: [String] = [], | |||
buildActionMask: UInt = 2147483647, |
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.
Yes!! No more magic ✨ numbers!
983ab18
to
65337f0
Compare
… be optional, optional
…ties that should be optionals
65337f0
to
8f46723
Compare
let proxyTypeString: String = try container.decode(.proxyType) | ||
self.proxyType = UInt(proxyTypeString).flatMap({ProxyType(rawValue: $0)}) ?? .other | ||
self.remoteGlobalIDString = try container.decode(.remoteGlobalIDString) | ||
let proxyTypeString: String? = try container.decodeIfPresent(.proxyType) |
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 noticed when the decoding changes happened there are a whole bunch of these cases where something is decoded into a string and then flatMaped into a UInt. Is there a specific reason for that, as opposed to decoding directly to a UInt?
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.
Does this not work?
self.proxyType = try container.decodeIfPresent(UInt.self, forKey: .proxyType).flatMap(ProxyType.init) ?? .other
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.
Yeah the decoder throws because doesn't know how to convert those values into integers @yonaskolb
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.
Ah ok, makes sense
Resolves #101
Short description 📝
Review which attributes should be optional or not.
Solution 📦
For each attribute, I remove it from the object and try to open the project with Xcode. If Xcode can handle not having the value, then the attribute is optional, if the project is considered invalid by Xcode, then the attribute is required.
Implementation
GIF