-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Rename $IsOSX to $IsMacOS #4757
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 a 8000 gree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I don't see changes to |
@daxian-dbw ok, looks like VSCode cached exclusion list and I previously excluded build.psm1, will fix. looks like .md files were excluded as well. |
5e51de6
to
a59c391
Compare
Apple Inc. uses |
@iSazonov we had that discussion in the issue, the consensus is that $IsMacOS is preferred over $IsmacOS for readability and other languages (like Java) use isMacOS, so it's already somewhat common. |
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
@@ -42,7 +42,7 @@ function Set-DailyBuildBadge | |||
$storageAccountKey = $Env:TestResultAccountKey | |||
|
|||
# this is the url referenced in README.MD which displays the badge | |||
$platform = if ( $IsOSX ) { "OSX" } else { "Linux" } | |||
$platform = if ( $IsLinux ) { "Linux" } else { "OSX" } |
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.
Minor comment - maybe replace "OSX" too?
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.
The file stored in Azure for the badge has OSX in the filename. I'm fine leaving that one.
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.
The package name for macOS should also be changed, but that can be a separate PR.
Should this run through the CI with [feature] to ensure none of the feature tests broke with the change? |
@markekraus good point, I'll resubmit |
a59c391
to
54a4786
Compare
AppVeyor failure is due to the current issue causing nightly to fail and @dantraMSFT is looking into it |
Yes, the failure is due to #4763. You can ignore it. |
54a4786
to
3331654
Compare
Rebased to pick up fix |
PS-Committee decided we should be consistent with our naming and conform to Apple's use of macOS instead of OSX, however, for readability and consistently we are staying with Pascal casing.
Also renamed instances of variations of
OSX
tomacOS
for consistency in comments/text (separate commit to make it easier to review)Fix #4700