Skip to content

Remove TS debugAdapter#11755

Merged
karthiknadig merged 3 commits into
microsoft:logging-changes-and-drop-old-debuggerfrom
karthiknadig:remove-ts-debug-adapter
May 12, 2020
Merged

Remove TS debugAdapter#11755
karthiknadig merged 3 commits into
microsoft:logging-changes-and-drop-old-debuggerfrom
karthiknadig:remove-ts-debug-adapter

Conversation

@karthiknadig

@karthiknadig karthiknadig commented May 11, 2020

Copy link
Copy Markdown
Member

For #7136

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@karthiknadig karthiknadig added the no-changelog No news entry required label May 11, 2020
@codecov-io

codecov-io commented May 11, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #11755 into logging-changes-and-drop-old-debugger will increase coverage by 0.17%.
The diff coverage is 89.47%.

Impacted file tree graph

@@                            Coverage Diff                            @@
##           logging-changes-and-drop-old-debugger   #11755      +/-   ##
=========================================================================
+ Coverage                                  60.72%   60.89%   +0.17%     
=========================================================================
  Files                                        629      620       -9     
  Lines                                      34007    33770     -237     
  Branches                                    4795     4767      -28     
=========================================================================
- Hits                                       20650    20565      -85     
+ Misses                                     12364    12199     -165     
- Partials                                     993     1006      +13
Impacted Files Coverage Δ
src/client/debugger/extension/adapter/factory.ts 93.87% <88.88%> (-2.56%) ⬇️
src/client/debugger/extension/types.ts 100.00% <100.00%> (ø)
src/client/ioc/serviceManager.ts 40.00% <0.00%> (-5.00%) ⬇️
src/client/logging/levels.ts 61.11% <0.00%> (ø)
src/client/logging/logger.ts 75.60% <0.00%> (ø)
src/client/logging/_global.ts 85.24% <0.00%> (ø)
src/client/logging/formatters.ts 53.57% <0.00%> (ø)
src/client/logging/transports.ts 81.25% <0.00%> (ø)
src/client/common/net/socket/socketServer.ts

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 063c33d...4cd2911. Read the comment docs.

DonJayamanne
DonJayamanne requested changes May 11, 2020
View reviewed changes

@DonJayamanne DonJayamanne left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please can we ensure we run the CI pipeline to ensure DS functional tests pass.
Thanks.
😢 long live TS debugger, lasted 4.5 years.
😄 less code, better debugger, finally things done right...was a long journey.

Comment thread gulpfile.js Outdated
DonJayamanne
DonJayamanne approved these changes May 12, 2020
View reviewed changes

@DonJayamanne DonJayamanne left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approving, I trust you'll remove Promise.all, not mandatory, but unnecessary code.

@sonarqubecloud

sonarqubecloud Bot commented May 12, 2020

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@karthiknadig karthiknadig changed the title [WIP]Remove TS debugAdapter Remove TS debugAdapter May 12, 2020
@karthiknadig karthiknadig marked this pull request as ready for review May 12, 2020 01:27
@kimadeline kimadeline mentioned this pull request May 12, 2020
Closed
8 tasks
@karthiknadig karthiknadig requested a review from kimadeline May 12, 2020 15:59
ericsnowcurrently
ericsnowcurrently approved these changes May 12, 2020
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

I left a comment about the deleted package.json entries, but I'll leave that to you to decide. 😄

Comment thread package.json
kimadeline
kimadeline approved these changes May 12, 2020
View reviewed changes
Comment thread gulpfile.js
@karthiknadig karthiknadig merged commit 2dd69fe into microsoft:logging-changes-and-drop-old-debugger May 12, 2020
@karthiknadig karthiknadig deleted the remove-ts-debug-adapter branch May 12, 2020 19:05
@lock lock Bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DonJayamanne DonJayamanne DonJayamanne approved these changes

+2 more reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

@kimadeline kimadeline kimadeline approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

no-changelog No news entry required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@karthiknadig @codecov-io @ericsnowcurrently @DonJayamanne @kimadeline