Remove '-ComputerName' from 'Get/Set/Remove-Service' #5094
Conversation
|
It's easier to review the changes after ignoring whitespaces: https://github.com/PowerShell/PowerShell/pull/5094/files?w=1 |
| NativeMethods.SERVICE_CONFIG_DESCRIPTION, | ||
| buffer); | ||
| // If '-Status' parameter is specified, do the necessary action to bring about the desired result | ||
|
|
There was a problem hiding this comment.
I do not like this comment. It says if -Status, do something to get what you want. Can we be more specific or remove this comment.
There was a problem hiding this comment.
Sure, will change it to // Handle '-Status' parameter.
| //stop service,give the force parameter always true as we have already checked for the dependent services | ||
| //Specify NoWait parameter as always false since we are not adding this switch to this cmdlet | ||
| DoStopService(service, true, true); | ||
| } |
There was a problem hiding this comment.
Since we are touching the code here, can we use named parameters?
| WriteObject(displayservice); | ||
| if (!service.Status.Equals(ServiceControllerStatus.Paused)) | ||
| //pause service | ||
| DoPauseService(service); |
There was a problem hiding this comment.
This comment is unnecessary.
There was a problem hiding this comment.
I didn't change this one. I will remove it.
| bool objServiceShouldBeDisposed = false; | ||
| try | ||
| if (InputObject != null) | ||
| { |
There was a problem hiding this comment.
This code looks very similar to line 1485. Can we refactor?
There was a problem hiding this comment.
They are from two different cmdlets: Set-Service (line 1485) and Remove-Service (2083)
|
@adityapatwardhan Your comments are addressed. Can you please take another look? |
| ServiceCommandException exception = | ||
| new ServiceCommandException(message, innerException); | ||
| exception.ServiceName = serviceName; | ||
| (null == innerException) ? "" : innerException.Message); |
There was a problem hiding this comment.
Can we use String.Empty?
There was a problem hiding this comment.
Please clarify for me: I found many "*" strings in cs files - should we create a constant for it?
| foreach (ServiceController service in OneService(pattern)) | ||
| ServiceController service = GetOneService(pattern); | ||
| if (service != null) | ||
| { |
There was a problem hiding this comment.
It seems GetOneService() never return null.
There was a problem hiding this comment.
When the service doesn't exist, accessing sc.Status; will throw InvalidOperationException. In that case the method will catch the exception and return null.
Fix #5090
Remove '-ComputerName' from 'Get/Set/Remove-Service'