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

Guard APIs for .NET Standard 2.1 #3167

Conversation

Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Mar 15, 2020

Follow up to #3131 (separate PR to keep things simpler).

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

The Guard APIs are in the Microsoft.Toolkit project, that currently only targets .NET Standard 2.0. This makes it necessary to always reference the System.Memory package, and it makes it impossible to decorate APIs with the new C# 8 nullability/flow attributes from .NET Core 3.0. This also results in a worse coding experience, as the code analysis tool don't have all the info it needs when invoking some calls (eg. Guard.IsNotNull won't notify it that the input variable will never be null afterwards).

What is the new behavior?

This PR makes a few changes, which don't alter the functionality at all or the API surface area. They're just changes in the project structure and code decorations. In particular:

  • Added .NET Standard 2.1 as an additional target for Microsoft.Toolkit
  • Made the System.Memory package only necessary when targeting .NET Standard 2.0. On .NET Standard 2.1, only the System.Runtime.CompilerServices.Unsafe package is referenced.
  • Added attributes (eg. [NotNull] and [DoesNotReturnWhen]) on some of the Guard APIs

Here's a screenshot showing the difference in the code analysis info when using the .NET Standard 2.0 and 2.1 versions of the same Guard.IsNotNull API (with the new attributes):

guard_net21

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@Sergio0694
Copy link
Member Author

@michael-hawker Thank you for the guidance with the merge, it worked! 😄🎉

@Sergio0694
Copy link
Member Author

@michael-hawker Added those overloads for Guard.IsTrue and Guard.IsFalse with a custom message in b67e5b8 as we discussed, and did some last minute refactoring/tweaks following the suggestions from the review in #3273. This PR should be good to go now! 😊

@michael-hawker
Copy link
Member

@azchohfi I can't find your old comment, we good?

@Sergio0694
Copy link
Member Author

@michael-hawker All resolved.
Previous review threads for future reference:

@michael-hawker michael-hawker dismissed azchohfi’s stale review May 15, 2020 21:13

Outdated, Comments Addressed

@michael-hawker michael-hawker merged commit 3910c2e into CommunityToolkit:master May 15, 2020
@michael-hawker michael-hawker added this to the 6.1 milestone May 15, 2020
@Sergio0694 Sergio0694 mentioned this pull request May 20, 2020
7 tasks
@Sergio0694 Sergio0694 mentioned this pull request Jun 4, 2020
7 tasks
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.

3 participants