Small refactorings #331
Small refactorings #331
Conversation
There was a problem hiding this comment.
PR Overview
This PR addresses issue #304 by ensuring that administrative privileges are correctly enforced, particularly on macOS. The changes refactor the execution flow in the uninstall command by moving the admin check to the start and combining the confirmation prompts.
- Moved the admin check to a common entry point for both yes option and interactive prompt flows.
- Combined the confirmation prompt conditions into a single if statement.
Reviewed Changes
| File | Description |
|---|---|
| src/dotnet-core-uninstall/Shared/Commands/UninstallCommandExec.cs | Refactored logic to ensure admin privileges are checked upfront and unified the confirmation prompt conditions. |
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/dotnet-core-uninstall/Shared/Commands/UninstallCommandExec.cs:43
- [nitpick] Consider extracting the combined condition into a clearly named boolean variable (e.g., 'shouldProceed') to improve readability and maintainability.
if (parseResult.FindResultFor(CommandLineConfigs.YesOption) != null || (AskItAndReturnUserAnswer(filtered) && AskWithWarningsForRequiredBundles(filtered)))
| var response = userResponse == null ? Console.ReadLine().Trim().ToUpper() : userResponse.ToUpper(); | ||
|
|
||
| var response = userResponse?.ToUpper() ?? Console.ReadLine().Trim().ToUpper(); | ||
|
|
||
| if (response.Equals("N")) |
There was a problem hiding this comment.
Should we be checking for NO as well, given that we check for y or yes? If we really want to restrict an interactive response, then I wonder if a loop with ReadKey is better and we only accept N/n/y/Y
There was a problem hiding this comment.
I was curious as to why it accepted 'y' and 'yes', but only 'n'.
If we were to change this, I would advocate towards dropping the 'yes' over adding the 'no', as I suspect most users would use the single character option (I know I would)
Addresses #304Small refactorings to
IsAdminand other places in the execute command.