Skip to content

Obliterate Logger class from existence#9844

Merged
karrtikr merged 4 commits into
microsoft:masterfrom
karrtikr:cleanup
Jan 31, 2020
Merged

Obliterate Logger class from existence#9844
karrtikr merged 4 commits into
microsoft:masterfrom
karrtikr:cleanup

Conversation

@karrtikr

@karrtikr karrtikr commented Jan 30, 2020 •
edited
Loading

Copy link
Copy Markdown

For #9807

  • 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.

@karrtikr karrtikr added the no-changelog No news entry required label Jan 30, 2020
@karrtikr karrtikr changed the title Remove all uses of Logger.logError() method Remove all uses of Logger methods and all DIs Jan 30, 2020
@codecov-io

codecov-io commented Jan 30, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #9844 into master will decrease coverage by 0.25%.
The diff coverage is 58.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9844      +/-   ##
==========================================
- Coverage   60.87%   60.62%   -0.26%     
==========================================
  Files         556      556              
  Lines       30043    30451     +408     
  Branches     4555     4665     +110     
==========================================
+ Hits        18290    18460     +170     
- Misses      10726    10964     +238     
  Partials     1027     1027
Impacted Files Coverage Δ
src/client/common/serviceRegistry.ts 100% <ø> (ø) ⬆️
src/client/common/types.ts 100% <ø> (ø) ⬆️
.../application/diagnostics/applicationDiagnostics.ts 94.87% <100%> (ø) ⬆️
...ent/interpreter/locators/services/pipEnvService.ts 82.97% <100%> (-0.18%) ⬇️
src/client/common/installer/productInstaller.ts 95.47% <100%> (-0.03%) ⬇️
src/client/common/logger.ts 79.62% <100%> (+6.85%) ⬆️
...ient/interpreter/locators/services/condaService.ts 85.39% <100%> (-0.09%) ⬇️
src/client/debugger/extension/banner.ts 89.62% <100%> (ø) ⬆️
...c/client/linters/errorHandlers/baseErrorHandler.ts 75% <100%> (-2.78%) ⬇️
src/client/datascience/codeCssGenerator.ts 8.37% <13.33%> (+0.52%) ⬆️
... and 22 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 77bc1f6...7e7c7eb. Read the comment docs.

@sonarqubecloud

sonarqubecloud Bot commented Jan 30, 2020

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
0.4% 0.4% Duplication

@karrtikr karrtikr changed the title Remove all uses of Logger methods and all DIs Obliterate Logger class and its interface Jan 30, 2020
@karrtikr karrtikr changed the title Obliterate Logger class and its interface Obliterate Logger class from existence Jan 30, 2020
kimadeline
kimadeline approved these changes Jan 31, 2020
View reviewed changes
Comment thread src/client/interpreter/locators/services/condaService.ts
// 1. conda is not installed.
// 2. `conda env list has changed signature.
this.logger.logInformation('Failed to get conda environment list from conda', ex);
traceError('Failed to get conda environment list from conda', ex);

@kimadeline kimadeline Jan 31, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

traceError not traceInfo?

@karrtikr karrtikr Jan 31, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes that's intentional. Good catch btw

@karrtikr karrtikr merged commit 34d8097 into microsoft:master Jan 31, 2020
@karrtikr karrtikr deleted the cleanup branch January 31, 2020 19:38
@kimadeline

kimadeline commented Jan 31, 2020

Copy link
Copy Markdown

@karrtikr did you get @DonJayamanne or anybody else from DS to take a look at it before merging it?

@karrtikr

karrtikr commented Jan 31, 2020

Copy link
Copy Markdown
Author

@kimadeline I asked Don

@lock lock Bot locked as resolved and limited conversation to collaborators Feb 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DonJayamanne DonJayamanne Awaiting requested review from DonJayamanne

@karthiknadig karthiknadig Awaiting requested review from karthiknadig

1 more reviewer

@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.

3 participants

@karrtikr @codecov-io @kimadeline