Skip to content

Change debug launch handler to treat null/empty cwd to not change dir #694

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 4, 2018
Merged

Change debug launch handler to treat null/empty cwd to not change dir #694

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f48cc1c
Change debug launch handler to treat null/empty cwd to not change dir
rkeithhill Jul 2, 2018
8ee61a3
Address PR feedback, reorg to not execute working dir code unless used
rkeithhill Jul 3, 2018
db17acb
Get rid of unnecessary pragma warning disable
rkeithhill Jul 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/PowerShellEditorServices.Protocol/DebugAdapter/LaunchRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,6 @@ public static readonly

public class LaunchRequestArguments
{
/// <summary>
/// Gets or sets the absolute path to the program to debug.
/// </summary>
[Obsolete("This property has been deprecated in favor of the Script property.")]
public string Program { get; set; }

/// <summary>
/// Gets or sets the absolute path to the script to debug.
/// </summary>
Expand Down
43 changes: 20 additions & 23 deletions src/PowerShellEditorServices.Protocol/Server/DebugAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,22 +255,9 @@ protected async Task HandleLaunchRequest(
this.RegisterEventHandlers();

// Set the working directory for the PowerShell runspace to the cwd passed in via launch.json.
// In case that is null, use the the folder of the script to be executed. If the resulting
// working dir path is a file path then extract the directory and use that.
string workingDir =
launchParams.Cwd ??
launchParams.Script ??
#pragma warning disable 618
launchParams.Program;
#pragma warning restore 618

// When debugging an "untitled" (unsaved) file - the working dir can't be derived
// from the Script path. OTOH, if the launch params specifies a Cwd, use it.
if (ScriptFile.IsUntitledPath(workingDir) && string.IsNullOrEmpty(launchParams.Cwd))
{
workingDir = null;
}
string workingDir = launchParams.Cwd;

// Assuming we have a specified working dir, unescape the path and verify it.
if (!string.IsNullOrEmpty(workingDir))
{
workingDir = PowerShellContext.UnescapePath(workingDir);
Expand All @@ -289,18 +276,28 @@ protected async Task HandleLaunchRequest(
}
}

if (string.IsNullOrEmpty(workingDir))
// A null or empty string value for CWD is an indicator to NOT change the working directory
// but only if we are debugging in the integrated console where there is an existing cwd.
// However, if we are running in a temporary integrated console, there is no pre-existing
// cwd, so fall through to the "else if: where we set a working directory.
if (string.IsNullOrEmpty(workingDir) && !launchParams.CreateTemporaryIntegratedConsole)
{
Logger.Write(LogLevel.Verbose, "Launch config requested working dir not be changed/set");
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
else if (this.editorSession.PowerShellContext.CurrentRunspace.Location == RunspaceLocation.Local &&
!this.editorSession.DebugService.IsDebuggerStopped)
{
// If we have no working dir by this point, pick some reasonable default.
if (string.IsNullOrEmpty(workingDir))
{
#if CoreCLR
workingDir = AppContext.BaseDirectory;
//TODO: RKH 2018-06-26 .NET standard 2.0 has added Environment.CurrentDirectory - let's use it.
Copy link
Member

Choose a reason for hiding this comment

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

soon.... :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://www.youtube.com/watch?v=39S3dLDEbYs

workingDir = AppContext.BaseDirectory;
#else
workingDir = Environment.CurrentDirectory;
workingDir = Environment.CurrentDirectory;
#endif
}
}

if (this.editorSession.PowerShellContext.CurrentRunspace.Location == RunspaceLocation.Local &&
!this.editorSession.DebugService.IsDebuggerStopped)
{
await editorSession.PowerShellContext.SetWorkingDirectory(workingDir, isPathAlreadyEscaped: false);
Logger.Write(LogLevel.Verbose, "Working dir set to: " + workingDir);
}
Expand All @@ -316,7 +313,7 @@ protected async Task HandleLaunchRequest(
// Store the launch parameters so that they can be used later
this.noDebug = launchParams.NoDebug;
#pragma warning disable 618
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the pragma can be removed -- 618 is to suppress "Obselete"

this.scriptToLaunch = launchParams.Script ?? launchParams.Program;
this.scriptToLaunch = launchParams.Script;
#pragma warning restore 618
this.arguments = arguments;
this.IsUsingTempIntegratedConsole = launchParams.CreateTemporaryIntegratedConsole;
Expand Down