Skip to content

Conversation

@buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Sep 9, 2021

Summary

Recently we updated the Platform Compatibility Analyzer to support a relationship of "MacCatalyst is a superset of IOS" using SupportedOSPlatformGuard attributes on the OperatingSystem method. We need to update the doc accordingly

Related PR: dotnet/roslyn-analyzers#5266

  // Macatalyst is a superset of iOS therefore supported on iOS and MacCatalyst  
  [SupportedOSPlatform("iOS")]
  public void ApiOnlySupportedOnIOSAndMacCatalyst() { }

  // Does not imply iOS, only supported on MacCatalyst
  [SupportedOSPlatform("MacCatalyst")]
  public void ApiOnlySupportedOnMacCatalyst() { }

  [SupportedOSPlatform("iOS")] // Supported on iOS and MacCatalyst  
  [UnsupportedOSPlatform("MacCatalyst")] // Removes implied MacCatalyst support
  public void ApiOnlySupportedOnIos() { }

  // Unsupported on iOS and MacCatalyst  
  [UnsupportedOSPlatform("iOS")]
  public void ApiUnsupportedOnIOSAndMacCatalyst();

  // Does not imply iOS, only unsupported on MacCatalyst
  [UnsupportedOSPlatform("MacCatalyst")]
  public void ApiUnsupportedOnMacCatalyst() { }

  [UnsupportedOSPlatform("iOS")] // Unsupported on iOS and MacCatalyst  
  [SupportedOSPlatform("MacCatalyst")] // Removes implied MacCatalyst unsupport
  public void ApiUnsupportedOnIos() { }

@gewarren gewarren changed the title Add doc for the latest "MacCatalyst is usperset of iOS" updates Add doc for the latest "MacCatalyst is superset of iOS" updates Sep 10, 2021
Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be good to include a table showing the support combinations. I'll see if I can come up with one.

Co-authored-by: Genevieve Warren <[email protected]>
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Sep 13, 2021

It might also be good to include a table showing the support combinations. I'll see if I can come up with one.

Thank you, i have updated the table bit and applied with all other suggestions

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one comment where I wasn't sure if the spacing/indentation was correct. Beyond that, the only feedback I have is that we're using a few different terms:

  1. Related
  2. Superset
  3. Parent/Child

If we're going to use parent/child, then "related" is a good word that refers to the relationship between parent/child. But I don't know that parent/child is the most intuitive or helpful term for the concept; I think superset/subset is more informative of the direction of the relationship.

If we had superset/subset instead of parent/child, then I'm not sure that "related" is best, but I also can't think of a good replacement.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Sep 18, 2021

If we're going to use parent/child, then "related" is a good word that refers to the relationship between parent/child. But I don't know that parent/child is the most intuitive or helpful term for the concept; I think superset/subset is more informative of the direction of the relationship.

If we had superset/subset instead of parent/child, then I'm not sure that "related" is best, but I also can't think of a good replacement.

Right, also i noticed that we used parent/child for nested attributes, like Assembly level attributes are parent for Type/API level attributes, etc. Therefore i updated the parent/child with superset/subset and instead related attributes used attributes relation. Please take another look

@gewarren
Copy link
Contributor

I left some suggestions in a PR since that was easier than line by line suggestions. buyaa-n#2

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Sep 30, 2021

I left some suggestions in a PR since that was easier than line by line suggestions. buyaa-n#2

Thank you @gewarren that is great, sorry for the late reply, the notification was lost within my emails, I just merged your PR.

@gewarren gewarren merged commit 3de8e22 into dotnet:main Sep 30, 2021
@buyaa-n buyaa-n deleted the maccatalyst-update branch October 1, 2021 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants