-
Notifications
You must be signed in to change notification settings - Fork 11
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 Obsolete warnings to all System.Data.SqlClient public APIs #158
Conversation
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.
While working on public APIs, the usage of those in internal classes also incurred CS0618 warnings for which I marked "SqlClient specific" internal APIs are obsolete too. Let me know for any concerns.
Isn't all System.Data.SqlClient supposed to be marked as obsolete? I was unable to understand the pattern of adding the Obsolete attribute to some specific APIs but not others. Why not add it at the assembly level?
Since we don't have .NET Standard functional asset, skipping check for that, would appreciate any feedback.
Can you please elaborate on that?
<TargetFrameworks>$(NetCurrent)-windows;$(NetCurrent)-unix;$(NetCurrent);$(NetMinimum)-windows;$(NetMinimum)-unix;$(NetMinimum);netstandard2.0;$(NetFrameworkMinimum)</TargetFrameworks> |
src/System.Data.SqlClient/src/Interop/SNINativeMethodWrapper.Common.cs
Outdated
Show resolved
Hide resolved
src/System.Data.SqlClient/src/Interop/SNINativeMethodWrapper.Windows.cs
Outdated
Show resolved
Hide resolved
src/System.Data.SqlClient/tests/Tools/TDS/TDS.Servers/Microsoft.SqlServer.TDS.Servers.xml
Outdated
Show resolved
Hide resolved
I looked up ObsoleteAttribute documentation and couldn't find any other way to mark APIS obsolete. Based on remarks I don't see they support at Assembly level. Let me know if you have a way to may it work. Regarding .Net Standard 2.0 target, yes we target the same but it's a PNSE assembly in the new package which doesn't compile these classes but the notsupported.cs file. I wasn't sure if we need to add the Obsolete attributes there. |
Yes, the .notsupported.cs file needs the Obsolete attributes as well. As Carlos mentioned, internal API doesn't need to be annotated with the attribute.
Correct, the |
Can't it be placed on top of a namespace? Edit: I checked, apparently it cannot be done. Oh well. |
4fc7130
to
4500ed8
Compare
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.
LGTM. I see now all public APIs have the attribute.
Thanks for removing the xmls.
It's a bummer that we can't localize the obsolete string.
Let's wait for any comments from @ViktorHofer , if any.
Addresses #125
Updated: