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

Replace [Obsolete] with [Experimental] attributes #1487

Open
wants to merge 4 commits into
base: release-1.16
Choose a base branch
from

Conversation

WhitWaldo
Copy link
Contributor

Description

There are some places where it makes sense to mark things as [Obsolete], but it'd be nice to reserve that attribute only for those places where something is truly obsolete and not recommended as a path going forward. It's all we've had to date to mark non-stable functionality, but with the shift to .NET 8 as the minimal version, this brings with it two new attributes: [Experimental] and [RequiresPreviewFeatures].

Unfortunately, [Experimental] seems to have something to do with code analyzers as it has a required string argument for the analyzer ID, so because I'm not entirely sure what this entails for the downstream user experience, I'm skipping over it.

Rather, I've replaced the pre-stable functionality previously marked with [Obsolete] with [RequiresPreviewFeatures] and retained the original message text, e.g. [RequiresPreviewFeatures("The API is currently not stable as it is in the Alpha stage. This attribute will be removed once it is stable.")]

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1219

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

Sorry, something went wrong.

…quiresPreviewFeatures] attribute where relevant

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo WhitWaldo self-assigned this Mar 17, 2025
@WhitWaldo WhitWaldo requested review from a team as code owners March 17, 2025 09:43
@WhitWaldo
Copy link
Contributor Author

@philliphoff It looks like [RequirePreviewFeature] requires that the csproj include the <EnablePreviewFeatures>true</EnablePreviewFeatures> and I don't think we want devs to have to opt into experimental compiler fun just for Dapr alpha/beta functionality.

[Experimental] similarly requires an analyzer ID be provided as it does something with code analyzers.

I haven't been able to identify any other attributes that quite flag a warning like [Obsolete] does. While I briefly contemplated just making a [Preview] attribute and an analyzer that would warn on it, that would require a separate package and I expect rather few people would actually install it (and that's rather beside the point).

To that end, I propose we keep [Obsolete] as-is and see if the .NET devs introduce a better option in a later release. Thoughts?

@philliphoff
Copy link
Collaborator

It looks like [RequirePreviewFeature] requires that the csproj include the true and I don't think we want devs to have to opt into experimental compiler fun just for Dapr alpha/beta functionality.

[Experimental] similarly requires an analyzer ID be provided as it does something with code analyzers.

@WhitWaldo The guidance says [RequirePreviewFeature] is intended primarily for components of the .NET Runtime itself. For most other things (in particular, 3rd-party libraries), one should use [Experimental] as this attribute is expressly intended to flag preview APIs in otherwise stable libraries. The diagnostic IDs just need to be reasonably unique/specific to the library as they serve both as a way to identify a specific feature/API as well as the means for disabling warnings/errors when intentionally used.

I would still suggest a switch to [Experimental] from [Obsolete].

@WhitWaldo
Copy link
Contributor Author

It looks like [RequirePreviewFeature] requires that the csproj include the true and I don't think we want devs to have to opt into experimental compiler fun just for Dapr alpha/beta functionality.

[Experimental] similarly requires an analyzer ID be provided as it does something with code analyzers.

@WhitWaldo The guidance says [RequirePreviewFeature] is intended primarily for components of the .NET Runtime itself. For most other things (in particular, 3rd-party libraries), one should use [Experimental] as this attribute is expressly intended to flag preview APIs in otherwise stable libraries. The diagnostic IDs just need to be reasonably unique/specific to the library as they serve both as a way to identify a specific feature/API as well as the means for disabling warnings/errors when intentionally used.

I would still suggest a switch to [Experimental] from [Obsolete].

I'll give it a shot and see what happens!

@WhitWaldo WhitWaldo changed the title Replace [Obsolete] with [RequiresPreviewFeatures] attributes Replace [Obsolete] with [Experimental] attributes Mar 17, 2025
…ntal] instead

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
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.

None yet

2 participants