-
-
Notifications
You must be signed in to change notification settings - Fork 278
Don't set process priority unless instructed to #282
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #282 +/- ##
==========================================
- Coverage 95.38% 95.23% -0.16%
==========================================
Files 48 48
Lines 1193 1197 +4
Branches 86 86
==========================================
+ Hits 1138 1140 +2
- Misses 35 37 +2
Partials 20 20 ☔ View full report in Codecov by Sentry. |
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.
PR Overview
This pull request ensures that process priority is only set when explicitly instructed, addressing issue #279. Key changes include:
- Rearranging the process startup sequence and error message construction in ProcessEx.cs.
- Adding a null check for ResourcePolicy.Priority and updating the exception message in Command.Execution.cs.
- Modifying ResourcePolicyBuilder.cs and ResourcePolicy.cs to support a nullable priority, preventing unintentional process priority settings.
Reviewed Changes
| File | Description |
|---|---|
| CliWrap/Utils/ProcessEx.cs | Moved start time recording and custom configuration post try-catch. |
| CliWrap/Command.Execution.cs | Added a null check for priority and updated the error message wording. |
| CliWrap/Builders/ResourcePolicyBuilder.cs | Updated priority type to nullable and adjusted setter accordingly. |
| CliWrap/ResourcePolicy.cs | Allowed a nullable priority by updating the default value and accessor. |
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
CliWrap/Command.Execution.cs:320
- [nitpick] Consider adding braces for the if-statement to improve clarity and reduce potential maintenance issues.
if (ResourcePolicy.Priority is not null)
* Don't set process priority unless instructed to * Improve exception message
Closes #279