-
Notifications
You must be signed in to change notification settings - Fork 458
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
Update default script phase values to match Xcode 10. #652
Update default script phase values to match Xcode 10. #652
Conversation
a08ec96
to
de6bdd1
Compare
# | ||
attribute :shell_script, String, '' | ||
attribute :shell_script, String, "# Type a script or drag a script file from your workspace to insert its path.\n" |
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'm not sure that Xcodeproj should care about imitating this behavior of Xcode. I assume this default exists mostly for Xcode UI discoverability/usability, but isn't terribly helpful for this gem. I don't feel strongly about this though.
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.
Its nuanced in a sense that if xcodeproj creates an empty script phase with defaults then Xcode will change it again therefore making any repo that is tracked by version control to be dirty.
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.
Although yes not many people in reality create empty script phases through Xcodeproj but its OK to fix this here.
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.
Are you sure that Xcode will take a project with an empty script phase and automatically add this comment to it? If so, that seems like bizarre behavior on their part to add content.
It makes sense for Xcode to remove the show_env_vars_in_log = 1
if the default behavior is to show environment variables when the setting isn't present.
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 it does at least with Xcode 10. It also adds it as a comment using #
and sets the value. Compared to Xcode 9 that just shows the same text without #
but does not set it as part of pbxproj
. I tested on both and this is why I changed it here for Xcode 10 and going forward.
# @note Defaults to true (`1`). | ||
# | ||
attribute :show_env_vars_in_log, String, '1' | ||
attribute :show_env_vars_in_log, String |
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.
Should something change such that setting this to '1' is equivalent to clearing it?
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 am not sure we want to add this logic here so much....I am making the PR in CocoaPods to ensure that this does not happen though
2a80987
to
ad2a4f5
Compare
Xcode 10 (actually Xcode 9 too) never sets
showEnvVarsInLog=1
inside apbxproj
even though the box is checked. For some reason Xcode 9 does not removeshowEnvVarsInLog
like Xcode 10 does though.Additionally Xcode 10 has a different default value that is a comment and transcribed into the
pbxproj
itself under the script phase.No need for a new test as its already covered.
Here's what Xcode 10 creates by default: