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.

Sign up for GitHub

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

Jump to bottom

Update StartListening method desciption #4344

Merged
merged 6 commits into from
Feb 21, 2018
Merged

Update StartListening method desciption #4344

merged 6 commits into from
Feb 21, 2018

Conversation

MSGaryWang
Copy link

@MSGaryWang MSGaryWang commented Feb 9, 2018 •
edited
Loading

The current description might mislead user calling this method unnecessarily, this change will help user understand clearly at the correct way to use this method.

Title

Update StartListening method description

Summary

Customer might use this API is a wrong way and lead to issues.

Details

Provide usage in details to prevent customer calling it unexpectedly.

@MSGaryWang
Update StartListening method desciption
ded6be9 
The current description might mislead user calling this method unnecessarily, this change will help user understand clearly at the correct way to use this method.
@rpetrusha
Copy link
Contributor

The build has failed for no detectable reason. Closing and reopening to begin new build.

@rpetrusha rpetrusha closed this Feb 9, 2018
@rpetrusha rpetrusha reopened this Feb 9, 2018
@mairaw
Copy link
Contributor

mairaw commented Feb 13, 2018

I've noticed the see cref tag was not properly closed. Let's see if this resolves the build issues.

rpetrusha
rpetrusha suggested changes Feb 13, 2018
View reviewed changes
Copy link
Contributor

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Thanks, @MSGaryWang, for providing this clarification. I have one suggested change to the first sentence of the Remarks section.

@MSGaryWang
Copy link
Author

@rpetrusha I didn't see your new change at Remark section.

rpetrusha
rpetrusha reviewed Feb 15, 2018
View reviewed changes
Copy link
Contributor

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

Sorry, @MSGaryWang. I'm not sure why my original comment disappeared.

xml/System.Runtime.Remoting.Channels.Tcp/TcpServerChannel.xml Outdated
<remarks>
<format type="text/markdown"><![CDATA[

## Remarks
It is not necessary to call this method to begin listening on a newly initialized channel.
The constructor of TcpServerChannel automatically calls StartListening() so one should not call this method unless <xref:System.Runtime.Remoting.Channels.Tcp.TcpServerChannel.StopListening%2A> was previously called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Better: "The <xref:System.Runtime.Remoting.Channels.Tcp.TcpServerChannel> constructor automatically calls `StartListening`, so you shouldn't call...

Copy link
Author

Choose a reason for hiding this comment

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

It looks good, please go ahead to make the change.

@mairaw mairaw merged commit 9604789 into dotnet:master Feb 21, 2018
@mairaw mairaw added this to the Sprint 131 (2/3/18 - 2/23/18) milestone Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpetrusha rpetrusha rpetrusha approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Sprint 131 (2/3/18 - 2/23/18)
Development

Successfully merging this pull request may close these issues.
3 participants
@MSGaryWang @rpetrusha @mairaw