Skip to content

Conversation

@kierenj
Copy link
Contributor

@kierenj kierenj commented Sep 12, 2022

Summary

The existing description implied that the Exited event could be raised "before" the user calls HasExited. This wasn't very clear in my opinion - so I've suggested a tweak to make it more explicit by revealing that the HasExited property accessor itself has a side-effect; invoking HasExited if the process has exited.

The existing description implied that the Exited event could be raised "before" the user calls HasExited. This wasn't very clear in my opinion - so I've suggested a tweak to make it more explicit by revealing that the HasExited property accessor itself has a side-effect; invoking HasExited if the process has exited.
@kierenj kierenj requested a review from a team as a code owner September 12, 2022 10:57
@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-System.Diagnostics.Process labels Sep 12, 2022
@ghost
Copy link

ghost commented Sep 12, 2022

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

Issue Details

Summary

The existing description implied that the Exited event could be raised "before" the user calls HasExited. This wasn't very clear in my opinion - so I've suggested a tweak to make it more explicit by revealing that the HasExited property accessor itself has a side-effect; invoking HasExited if the process has exited.

Author: kierenj
Assignees: -
Labels:

area-System.Diagnostics.Process, community-contribution

Milestone: -

@kierenj kierenj changed the title Clarify Process.HasExited and the it raises Exited Clarify Process.HasExited and that it raises Exited Sep 12, 2022
@opbld34
Copy link

opbld34 commented Sep 12, 2022

Docs Build status updates of commit 1af1cb4:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Diagnostics/Process.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM. Thanks!

@jozkee jozkee enabled auto-merge (squash) October 11, 2022 20:16
@opbld32
Copy link

opbld32 commented Oct 11, 2022

Learn Build status updates of commit bb90361:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Diagnostics/Process.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@jozkee jozkee merged commit 26cfd3f into dotnet:main Oct 11, 2022
@kierenj kierenj deleted the patch-1 branch October 12, 2022 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Diagnostics.Process community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants