Skip to content
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

Jump to bottom

Change how debug options are captured in launch.json #1395

Merged
merged 18 commits into from
Apr 18, 2018
Merged

Change how debug options are captured in launch.json #1395

merged 18 commits into from
Apr 18, 2018

Conversation

DonJayamanne
Copy link

@DonJayamanne DonJayamanne commented Apr 13, 2018 •
edited
Loading

Fixes #1326

This pull request:

  • Has a title summarizes what is changing
  • Includes a news entry file (remember to thank yourself!)
  • Has unit tests & code coverage is not adversely affected (within reason)
  • Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
  • Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)

@codecov
Copy link

codecov bot commented Apr 13, 2018 •
edited
Loading

Codecov Report

Merging #1395 into master will decrease coverage by 0.22%.
The diff coverage is 80.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1395      +/-   ##
==========================================
- Coverage   70.87%   70.65%   -0.23%     
==========================================
  Files         271      271              
  Lines       12489    12514      +25     
  Branches     2213     2220       +7     
==========================================
- Hits         8852     8842      -10     
- Misses       3499     3534      +35     
  Partials      138      138
Impacted Files Coverage Δ
src/client/debugger/Common/Contracts.ts 100% <ø> (ø) ⬆️
.../client/debugger/DebugServers/RemoteDebugServer.ts 8.13% <0%> (ø) ⬆️
src/client/extension.ts 96.09% <100%> (ø) ⬆️
.../client/debugger/configProviders/pythonProvider.ts 100% <100%> (ø) ⬆️
...client/debugger/DebugClients/localDebugClientV2.ts 66.66% <100%> (+2.38%) ⬆️
...rc/client/debugger/configProviders/baseProvider.ts 93.9% <100%> (-0.08%) ⬇️
...c/client/debugger/DebugClients/launcherProvider.ts 80% <33.33%> (-20%) ⬇️
.../client/debugger/DebugClients/RemoteDebugClient.ts 35% <33.33%> (ø) ⬆️
src/client/debugger/Main.ts 51.85% <66.66%> (ø) ⬆️
...lient/debugger/configProviders/pythonV2Provider.ts 86.27% <80.55%> (-13.73%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41ec24a...dec303a. Read the comment docs.

MikhailArkhipov
MikhailArkhipov reviewed Apr 15, 2018
View reviewed changes
src/client/debugger/Common/Contracts.ts
console?: 'none' | 'integratedTerminal' | 'externalTerminal';
port?: number;
host?: string;
logToFile?: boolean;
}

export interface AttachRequestArguments extends DebugProtocol.AttachRequestArguments {
export interface LaunchRequestArgumentsV1 extends BaseLaunchRequestArguments {
Copy link

Choose a reason for hiding this comment

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

Should it be ISomething?

Copy link
Author

Choose a reason for hiding this comment

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

We are not creating classes based on these interfaces (hence no I). This was done before types came along

MikhailArkhipov
MikhailArkhipov reviewed Apr 15, 2018
View reviewed changes
src/client/debugger/DebugClients/DebugFactory.ts Outdated
if (launchRequestOptions.noDebug === true) {
return new NonDebugClient(launchRequestOptions, debugSession, canLaunchTerminal, new NoDebugLauncherScriptProvider());
launchScriptProvider = launchRequestOptions.type === 'pythonExperimental' ? new NoDebugLauncherScriptProviderV2() : new NoDebugLauncherScriptProvider();
Copy link

Choose a reason for hiding this comment

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

having value as string is somewhat error prone (to typos). Perhaps make pythonExperimental enum or constant?

Copy link
Author

Choose a reason for hiding this comment

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

It is type checked (that property only allows two vales of strings).
So typos will be caught by compiler. (String literal types)

MikhailArkhipov
MikhailArkhipov reviewed Apr 15, 2018
View reviewed changes
src/client/debugger/configProviders/pythonV2Provider.ts Outdated
debugConfiguration.debugOptions.push(DebugOptions.Jinja);
&& debugOptions.indexOf(DebugOptions.Jinja) === -1
&& debugConfiguration.jinja !== false) {
debugOptions.push(DebugOptions.Jinja);
Copy link

Choose a reason for hiding this comment

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

OK to push twice?

Copy link
Author

Choose a reason for hiding this comment

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

No problem, besides, this code is only for backwards compatibility (well remove this code soon).

@DonJayamanne DonJayamanne changed the title WIP - Change how debug options are captured in launch.json Change how debug options are captured in launch.json Apr 17, 2018
@DonJayamanne DonJayamanne force-pushed the issue1326DebugOptions branch 2 times, most recently from d2c9204 to d4d1de8 Compare April 17, 2018 23:24
@DonJayamanne DonJayamanne merged commit 6c17234 into microsoft:master Apr 18, 2018
@brettcannon
Copy link
Member

@DonJayamanne any specific reason not to put in a news entry for this change?

@DonJayamanne
Copy link
Author

This applies to experimental debugger, didn't see the need.
On second thought it does, as it's a change compared to the old debugger.

@DonJayamanne DonJayamanne deleted the issue1326DebugOptions branch June 20, 2018 03:14
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikhailArkhipov MikhailArkhipov MikhailArkhipov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overriding default debugOptions in launch.json
3 participants
@DonJayamanne @brettcannon @MikhailArkhipov