Change debug launch handler to treat null/empty cwd to not change dir #694
Conversation
Fix for PowerShell/vscode-powershell#1330 Removes deprecated "Program" field from LaunchRequest class. Simplifies working dir logic in HandleLaunchRequest. Basically if the launch request happens in the regular integrated console, a null/empty cwd means "do not change the working dir". If the request is using the temp integrated console, there is no "existing" workng dir, so we use the original logic to set the working dir.
|
BTW please thoroughly review this one. We are changing the way we handle a launch request with a null or empty |
| { | ||
| Logger.Write(LogLevel.Verbose, "Launch config requested working dir not be changed/set"); |
There was a problem hiding this comment.
Doesn't look like this if is doing anything besides logging... maybe refactor to just an if statement and then after the if log the location of the workingDir?
There was a problem hiding this comment.
Well, in the case above we don't know or care about the location of the working dir but we can simplify the logic and put in a single log call. Inside that log call I can use a ternary to determine what to log.
| { | ||
| workingDir = Path.GetDirectoryName(workingDir); | ||
| if ((File.GetAttributes(workingDir) & FileAttributes.Directory) != FileAttributes.Directory) |
There was a problem hiding this comment.
what is this magical syntax?
File.GetAttributes(workingDir) & FileAttributes.Directory
There was a problem hiding this comment.
Enums as flags. Attributes are usually modelled as flag enums by C# to make them more readable.
There was a problem hiding this comment.
I usually see the check as != 0, but another recommendation I've seen is to use HasFlag():
if (File.GetAttributes(workingDir).HasFlag(FileAttributes.Directory))
{
...
}There was a problem hiding this comment.
Apparently HasFlag() was added in .NET 4.
| if (string.IsNullOrEmpty(workingDir)) | ||
| // If we have no working dir by this point and we are running in a temp console, | ||
| // pick some reasonable default. | ||
| if (string.IsNullOrEmpty(workingDir) && launchParams.CreateTemporaryIntegratedConsole) | ||
| { | ||
| #if CoreCLR | ||
| //TODO: RKH 2018-06-26 .NET standard 2.0 has added Environment.CurrentDirectory - let's use it. |
There was a problem hiding this comment.
| @@ -316,7 +317,7 @@ protected void Stop() | |||
| // Store the launch parameters so that they can be used later | |||
| this.noDebug = launchParams.NoDebug; | |||
| #pragma warning disable 618 | |||
There was a problem hiding this comment.
Looks like the pragma can be removed -- 618 is to suppress "Obselete"
Fix for PowerShell/vscode-powershell#1330
Removes deprecated "Program" field from LaunchRequest class.
Simplifies working dir logic in HandleLaunchRequest. Basically if the
launch request happens in the regular integrated console, a null/empty
cwd means "do not change the working dir". If the request is using the
temp integrated console, there is no "existing" workng dir, so we use
the original logic to set the working dir.