-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Sunday cleanup project: replace internal usage analyzer with [Experimental] #33385
Conversation
Note: I am very strongly against replacing our specific pubternal attribute, which is for one thing, with Experimental, which is for another thing. |
@@ -231,10 +231,12 @@ public static DbCommand CreateDbCommand(this IQueryable source) | |||
public static IQueryable<TEntity> AsSingleQuery<TEntity>( | |||
this IQueryable<TEntity> source) | |||
where TEntity : class | |||
#pragma warning disable EF9901 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of a scenario missed by the analyzer (is checks)
@@ -1937,12 +1939,14 @@ protected override Expression VisitConditional(ConditionalExpression conditional | |||
// jsonManagerPrm.CaptureState(); | |||
for (var i = 0; i < 5; i++) | |||
{ | |||
#pragma warning disable EF9901 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another example of a case missed by the analyzer - typeof
I must have misunderstood - I thought we raised this possibility and you were in favor. I'll bring this to design. |
I'm in favor of using Experimental when we have API that is intended to be used by applications, but needs time to bake. (We occasionally make such API pubternal today.) But I believe that we need to be as explicit and in-your-face as possible with the pubternal messaging, since it's an unusual aspect of our code base that I think it's important people don't accidentally miss. |
Just to provide context, someone using an API annotated with
At the end of the day, the user doesn't really get exposed to the name of the attribute - it could be called anything; the important thing here IMHO is that we have a standard, reliable mechanism for generating a compilation warning that doesn't involve us maintaing our own analyzer. The message seems close enough to me, with the important part again being "subject to change or removal in future updates". The user is very explicitly getting an in-your-face message. If we believe the semantic difference in the un-exposed name ("experimental" vs. "internal") is important enough to keep maintaining our own analyzer mechanism, well, that's not going to be a hill II'm going to die on... (Note also that the diagnostic ID is specified as part of the attribute, and a URL can point the user at specific documentation as well, clarifying the situation etc.) |
can we hold off with this one at least until #33351 is checked in? |
Sure |
Closing as per design decision 😢 |
Some notes (I can bring this to a design discussion):
Possibly future directions:
Closes #33384