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.

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

Displayed actionable message when LS is not supported #3864

Merged
merged 9 commits into from
Jan 9, 2019

Conversation

karrtikr
Copy link

@karrtikr karrtikr commented Jan 3, 2019

For #3634

  • 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!)
  • 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)

@karrtikr karrtikr requested a review from DonJayamanne January 3, 2019 23:23
@ericsnowcurrently ericsnowcurrently changed the title Displayed actionable message when LS is not supported [WIP] Displayed actionable message when LS is not supported Jan 4, 2019
@codecov
Copy link

codecov bot commented Jan 4, 2019

Codecov Report

Merging #3864 into master will increase coverage by 3%.
The diff coverage is 89%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #3864    +/-   ##
=======================================
+ Coverage      78%     81%    +3%     
=======================================
  Files         401     400     -1     
  Lines       18516   18317   -199     
  Branches     2977    2953    -24     
=======================================
+ Hits        14385   14740   +355     
+ Misses       4129    3574   -555     
- Partials        2       3     +1
Flag Coverage Δ
#Linux 68% <62%> (ø) ⬇️
#Windows 68% <62%> (ø) ⬇️
#macOS 67% <62%> (ø) ⬇️
Impacted Files Coverage Δ
src/client/application/diagnostics/constants.ts 100% <100%> (ø) ⬆️
src/client/activation/activationService.ts 97% <100%> (+1%) ⬆️
src/client/common/utils/localize.ts 97% <100%> (ø) ⬇️
...t/application/diagnostics/checks/lsNotSupported.ts 86% <86%> (ø)
...rc/client/common/errors/moduleNotInstalledError.ts 34% <0%> (-66%) ⬇️
src/client/linters/errorHandlers/standard.ts 40% <0%> (-55%) ⬇️
...c/client/linters/errorHandlers/baseErrorHandler.ts 78% <0%> (-22%) ⬇️
src/client/linters/errorHandlers/errorHandler.ts 78% <0%> (-22%) ⬇️
src/client/linters/errorHandlers/notInstalled.ts 45% <0%> (-16%) ⬇️
src/client/interpreter/autoSelection/proxy.ts 87% <0%> (-13%) ⬇️
... and 46 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 cefe710...834f5a1. Read the comment docs.

@karrtikr karrtikr changed the title [WIP] Displayed actionable message when LS is not supported Displayed actionable message when LS is not supported Jan 4, 2019
d3r3kk
d3r3kk previously requested changes Jan 5, 2019
src/client/activation/activationService.ts Outdated Show resolved Hide resolved
src/client/activation/activationService.ts Outdated Show resolved Hide resolved
src/test/activation/activationService.unit.test.ts Outdated Show resolved Hide resolved
@karrtikr karrtikr force-pushed the LSbutton branch 4 times, most recently from 556b0ab to ce16f5f Compare January 5, 2019 03:35
src/client/activation/activationService.ts Outdated Show resolved Hide resolved
src/client/activation/activationService.ts Outdated Show resolved Hide resolved
src/client/activation/activationService.ts Outdated Show resolved Hide resolved
src/client/activation/activationService.ts Show resolved Hide resolved
Copy link

@d3r3kk d3r3kk left a comment

Choose a reason for hiding this comment

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

Looks good. One slight adjustment you could/should make and please follow up with @DonJayamanne on the injection thing, then you should be g2g!

src/client/activation/activationService.ts Outdated Show resolved Hide resolved
Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

All the other diagnostic files follow the same pattern (they do not inject messageService). Should I still do it?

Yes

@DonJayamanne
Copy link

Also, @karrtikr you need to fix the merge conflicts

@karrtikr karrtikr closed this Jan 8, 2019
@karrtikr karrtikr reopened this Jan 8, 2019
@karrtikr karrtikr closed this Jan 8, 2019
@karrtikr karrtikr reopened this Jan 8, 2019
@karrtikr karrtikr merged commit 4c2655f into microsoft:master Jan 9, 2019
@karrtikr karrtikr deleted the LSbutton branch January 9, 2019 08:45
@lock lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants