Skip to content
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

OSX needs to treated as the same platform as MacOS, everywhere #5012

Merged
merged 4 commits into from
Apr 13, 2021

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Apr 11, 2021

OSX needs to treated as the same platform as MacOS, everywhere

Somehow the code handling this logic lost 🙅 (i remember had it handled in my first version, I guess it is lost in the design change refactoring result).

The brute force way for treating OSX as MacOS would add additional check for each OS platform check and platform lookup, which don't like to do. So instead:

  • Whenever an OSPlatform attribute with OSX found for an API adding it as MacOs in the internal cache, so no need to add OSX specific special handling anymore
  • The only issue with this solution can be seen in reporting the incompatible APIs, i.e. when there is OSX specific API used from an incompatible call site then developers would get a warning that the API is supported on MacOS instead of OSX. Handling this issue with reporting the platform as MacOS/OSX - meaning it can be MacOS or OSX

Example:

class Test
{
    public void Api_Usage()
    {
        if (OperatingSystem.IsWindows() ||
            OperatingSystem.IsLinux() ||
            OperatingSystem.IsMacOS())
        {
            Api();
            Api2(); // All supported platforms guarded
        }

        [|Api()|]; // This call site is reachable on all platforms. 'Test.Api()' is only supported on: 'MacOS/OSX', 'Linux', 'windows'.
        [|Api2()|]; // This call site is reachable on all platforms. 'Test.Api2()' is only supported on: 'MacOS/OSX', 'Linux', 'windows'.
    }

    [SupportedOSPlatform("macos")]
    [SupportedOSPlatform("windows")]
    [SupportedOSPlatform("Linux")]
    void Api() { }

    [SupportedOSPlatform("windows")]
    [SupportedOSPlatform("Osx")]
    [SupportedOSPlatform("Linux")]
    void Api2() { }
}

@buyaa-n buyaa-n requested a review from a team as a code owner April 11, 2021 23:37
@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #5012 (e91175a) into release/6.0.1xx-preview4 (1dcfb32) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@                     Coverage Diff                      @@
##           release/6.0.1xx-preview4    #5012      +/-   ##
============================================================
- Coverage                     95.57%   95.54%   -0.04%     
============================================================
  Files                          1177     1188      +11     
  Lines                        269979   273151    +3172     
  Branches                      16362    16537     +175     
============================================================
+ Hits                         258038   260984    +2946     
- Misses                         9810     9976     +166     
- Partials                       2131     2191      +60     

@buyaa-n buyaa-n merged commit 135e126 into dotnet:release/6.0.1xx-preview4 Apr 13, 2021
@buyaa-n buyaa-n deleted the osx_same_as_macos branch April 13, 2021 19:38
@jmarolf jmarolf mentioned this pull request Aug 16, 2021
This was referenced Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants