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

Add description to RequiresPreviewFeaturesAttribute #56498

Closed
JamesNK opened this issue Jul 28, 2021 · 6 comments · Fixed by #56938
Closed

Add description to RequiresPreviewFeaturesAttribute #56498

JamesNK opened this issue Jul 28, 2021 · 6 comments · Fixed by #56938
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Jul 28, 2021

It would be nice if there was a way to provide a description with RequiresPreviewFeaturesAttribute, like you can with ObsoleteAttribute. The description would show up in analyzers/build warnings.

A description would provide more information about why a feature is in preview and what a user should keep in mind if they choose to use the feature.

For example, HTTP/3 in .NET 6 is preview. Kind of detail we want to pass on when someone sees this warning:

  • What scenarios HTTP/3 does and doesn't support in .NET 6.
  • How to mitigate an app if HTTP/3 is turned on and problems are found.

API Proposal

namespace System.Runtime.Versioning
{
    public sealed class RequiresPreviewFeaturesAttribute : Attribute
    {
        // Existing
        // public RequiresPreviewFeaturesAttribute();
        public RequiresPreviewFeaturesAttribute(string message);
        public string Message { get; }
        public string Url { get; set; }
    }
}

Note: The URL property allows us to surface the URL directly via the Roslyn diagnostic, so that people can click the diagnostic ID in the error list, rather than copy & pasting the URL.

API Usage

 namespace Microsoft.AspNetCore.Server.Kestrel.Core
 {
     [Flags]
     public enum HttpProtocols
     {
         Http1 = 0x1,
         Http2 = 0x2,
         Http1AndHttp2 = Http1 | Http2,
+        [RequiresPreviewFeatures("Kestrel HTTP3 support for .NET 6 is in preview", Url = "https://aka.ms/kestrel-http3")]
         Http3 = 0x4,
+        [RequiresPreviewFeatures("Kestrel HTTP3 support for .NET 6 is in preview", Url = "https://aka.ms/kestrel-http3")]
         Http1AndHttp2AndHttp3 = Http1 | Http2 | Http3
     }
 }
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jul 28, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@stephentoub
Copy link
Member

@stephentoub stephentoub added this to the 6.0.0 milestone Jul 29, 2021
@ghost
Copy link

ghost commented Jul 29, 2021

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

It would be nice if there was a way to provide a description with RequiresPreviewFeaturesAttribute, like you can with ObsoleteAttribute. The description would show up in analyzers/build warnings.

A description would provide more information about why a feature is in preview and what a user should keep in mind if they choose to use the feature.

For example, HTTP/3 in .NET 6 is preview. Kind of detail we want to pass on when someone sees this warning:

  • What scenarios HTTP/3 does and doesn't support in .NET 6.
  • How to mitigate an app if HTTP/3 is turned on and problems are found.
Author: JamesNK
Assignees: -
Labels:

area-System.Runtime, untriaged

Milestone: 6.0.0

@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Jul 29, 2021
@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jul 29, 2021
@terrajobst
Copy link
Member

@JamesNK I've added the API proposal and sample information and marked the issue as ready for review. We'll take a look next Tuesday, I'll send you an invite on Monday.

@JamesNK
Copy link
Member Author

JamesNK commented Jul 30, 2021

Ok. I might not make it because of time zones, but I think the issue is pretty self explanatory.

@terrajobst
Copy link
Member

terrajobst commented Aug 3, 2021

Video

  • We discussed whether or not the analyzer should also offer a diagnostic ID, akin to ObsoleteAttribute
    • We're leaning towards no, because that seems to re-introduce the feature names that we considered in the design and decided against due to complexity and test matrix concerns
    • That means we wouldn't have UrlFormat but just Url
namespace System.Runtime.Versioning
{
    public sealed class RequiresPreviewFeaturesAttribute : Attribute
    {
        // Existing
        // public RequiresPreviewFeaturesAttribute();
        public RequiresPreviewFeaturesAttribute(string? message);
        public string? Message { get; }
        public string? Url { get; set; }
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 3, 2021
@pgovind pgovind self-assigned this Aug 5, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 5, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 9, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants