-
-
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
Added useTestSelectionWhitelist attribute to XCScheme.TestableReference #516
Conversation
@@ -8,6 +8,7 @@ extension XCScheme { | |||
public var skipped: Bool | |||
public var parallelizable: Bool | |||
public var randomExecutionOrdering: Bool | |||
public var useTestSelectionWhitelist: Bool |
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.
Hi @basvankuijck. Thanks for opening this PR. Would you mind making this attribute optional? Since Xcode is able to open projects where this attribute is not defined that means that the attribute is optional and we should reflect it as such.
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.
Hi @pepibumur, I don’t understand your reasoning? If this property is not in a project it will be set to false when reading. When writing it will only be set if true. This is the same for the other booleans in this class and many others
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.
If a read project has that property as nil, we should keep it as nil to write it back as nil. If we turn nil into a default boolean, we’ll end up creating an attribute when we shouldn’t. That causes diff issues that users will end up reporting an issues.
It’s not an issue when generating projects, but it is when reading and writing existing projects.
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.
In other words, nil != 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.
If a project is read where this is nil, then the property can be set to false. When the project is written again, the value won't be written if the value is false, meaning it will end up the same. This causes 0 diffs.
The only theoretical diff is if a project had this explicitly set to false, then reading and writing it would remove the property. This wouldn't happen though as Xcode removes the property when you set it to false in the UI, which is what we are proposing XcodeProj do as well
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.
This is also the same logic that the other bools in this class have been using for years
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.
This wouldn't happen though as Xcode removes the property when you set it to false in the UI, which is what we are proposing XcodeProj do as well
Xcode writes are optimized for minimizing the diffs after writing the project. If we read, we write nil and as a user of the tool there are no unexpected diffs. We might have missed this in some other properties.
I don’t see what’s the problem with nit changing what we read from the disk.
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.
Just a minor comment regarding the optionality of the value. Once addressed, and updated the CHANGELOG, I think we can go ahead and merge the PR.
d6ebbda
to
912d40c
Compare
The changes should be on master now. I'm releasing a new version of XcodeProj that includes the new attribute. Thanks for adding this feature @basvankuijck |
Short description 📝
Add the
useTestSelectionWhitelist
attribute to scheme creation so it can either automatically include (or exclude) new tests