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

Obsolete Socket.UseOnlyOverlappedIO #52475

Merged
merged 6 commits into from
May 18, 2021
Merged

Conversation

hrrrrustic
Copy link
Contributor

close #47163

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented May 7, 2021

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

Issue Details

close #47163

Author: hrrrrustic
Assignees: -
Labels:

area-System.Net.Sockets, new-api-needs-documentation

Milestone: -

using Socket s = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);

Assert.False(s.UseOnlyOverlappedIO);
s.UseOnlyOverlappedIO = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep some of the tests around, with the warning disabled around the use in tests.

We need to make sure that it still works as expected even once it is obsolete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I add pragma warning disable without diagnstic id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I can use CS0618
Okay, I'll return tests 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time for this! We need to include .NET 5+ in the obsoletion messages, otherwise looks good.

hrrrrustic and others added 2 commits May 11, 2021 18:02
Co-authored-by: Anton Firszov <antonfir@gmail.com>
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@antonfirsov antonfirsov merged commit 7be77b8 into dotnet:main May 18, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@@ -292,6 +292,8 @@ public partial class Socket : System.IDisposable
[System.ObsoleteAttribute("SupportsIPv6 is obsoleted for this type, please use OSSupportsIPv6 instead. https://go.microsoft.com/fwlink/?linkid=14202")]
public static bool SupportsIPv6 { get { throw null; } }
public short Ttl { get { throw null; } set { } }
[System.ObsoleteAttribute("This property has no effect in .NET 5+ and .NET Core.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@terrajobst, we really need a better way to refer to this :) For anything that started off in .NET Core, are we now going to have to say .NET Core and .NET 5+?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The official guide suggests to refer to it as

".NET" (without mention of 5 or Core)

which would be terribly confusing in this case. ("This property has no effect in .NET" 🤷)

Copy link
Member

@stephentoub stephentoub May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be terribly confusing in this case

Right, but so is ".NET 5+ and .NET Core", as it incorrectly implies that .NET 5 isn't .NET Core.

cc: @richlander

Copy link
Member

@jkotas jkotas May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

".NET" (without mention of 5 or Core)

When you need to emphasize specific implementation (the case here), the guide suggests to use ".NET 5+ (and .NET Core)" for first reference and ".NET 5+" for subsequent references.

This guide was created just recently. I did not know about that. @antonfirsov thank you for sharing the link. There is a ton of places everywhere that use the phrase like the one used by this PR that was de-facto standard before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the guide suggests to use ".NET 5+ (and .NET Core)"

Actually, it suggests:

.NET 5 (and .NET Core) and later versions

which is terribly verbose (at a level that makes it obscure IMO), this is why I went with .NET 5+ and .NET Core. I believe we should revisit that guide.

Right, but so is ".NET 5+ and .NET Core", as it incorrectly implies that .NET 5 isn't .NET Core.

@stephentoub although from internal (dotnet/runtime developer) perspective .NET 5 ⊂ .NET Core (being just another release), from what I see from the guide, this is not what's being communicated on the branding front.

Maybe we should open a tracking issue, to revisit the terminology? If yes where?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.NET 5 (and .NET Core) and later versions

"and later versions" is the standardized docs terminology. Yes, it is verbose and it is often abbreviated. Perhaps the guide should mention the .NET 5+ abbreviation as an option. For example, this abbrevation is often used in https://github.com/dotnet/dotnet-api-docs currently.

Maybe we should open a tracking issue, to revisit the terminology? If yes where?

The docs repo. The document that describes the terminology is there.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Obsolete Socket.UseOnlyOverlappedIO
5 participants