-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
Fix target attribute optionality #519
Conversation
@yonaskolb your pull request is missing a changelog! |
guard let targetAttributeReferences = targetAttributeReferences, | ||
let rhsTargetAttributeReferences = rhs.targetAttributeReferences, | ||
NSDictionary(dictionary: targetAttributeReferences).isEqual(to: rhsTargetAttributeReferences) else { return false } | ||
if !NSDictionary(dictionary: targetAttributeReferences).isEqual(to: rhs.targetAttributeReferences) { return false } |
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.
Fixes incorrect case were both were nil
Awesome! Thanks for tackling this.
The rule we have been following to decide whether an attribute should be or not optional is that the attribute should be optional if Xcode is able to open a project with the attribute not define. That's the case for |
As per #517 though, this property is always written so that Fastlane can open it. There's no room for it to be optional |
Although I disagree with going down that path, it's not a big deal so let's merge this and revisit it before the next major version to avoid breaking changes. |
I'll release a new version right after the merge |
Thank you @pepibumur! :) |
#517 made a breaking change to make
targetAttributes
optional.This PR fixes:
targetAttributes
were optionaltargetAttributes
non optional, as they are always set in both the initializers. The attributes are always still written which solves the aim of Make target attributes optional #517