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

Show the prompt again if user clicks on more info #11664

Merged
merged 3 commits into from
May 7, 2020

Conversation

karthiknadig
Copy link
Member

For #11648

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

Skipped news since this is a feature not yet turned on for users.

@karthiknadig karthiknadig added the no-changelog No news entry required label May 7, 2020
@karthiknadig karthiknadig requested review from int19h and karrtikr May 7, 2020 03:02
@codecov-io
Copy link

codecov-io commented May 7, 2020

Codecov Report

Merging #11664 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11664      +/-   ##
==========================================
+ Coverage   60.60%   60.65%   +0.05%     
==========================================
  Files         627      627              
  Lines       33865    33878      +13     
  Branches     4763     4765       +2     
==========================================
+ Hits        20523    20549      +26     
+ Misses      12355    12335      -20     
- Partials      987      994       +7     
Impacted Files Coverage Δ
...ication/diagnostics/checks/pythonPathDeprecated.ts 96.15% <ø> (-0.08%) ⬇️
src/client/common/utils/localize.ts 95.53% <ø> (ø)
src/datascience-ui/react-common/arePathsSame.ts 75.00% <0.00%> (-12.50%) ⬇️
src/client/common/utils/platform.ts 64.70% <0.00%> (-11.77%) ⬇️
src/client/linters/pydocstyle.ts 86.66% <0.00%> (-2.23%) ⬇️
src/client/datascience/debugLocationTracker.ts 76.56% <0.00%> (-1.57%) ⬇️
src/client/common/process/proc.ts 14.49% <0.00%> (-0.73%) ⬇️
src/datascience-ui/common/index.ts 94.21% <0.00%> (+1.65%) ⬆️
...tascience/interactive-ipynb/nativeEditorStorage.ts 55.97% <0.00%> (+7.04%) ⬆️

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 d816dc5...568c5d2. Read the comment docs.

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.

I think a better way is to add a link in the message instead
Click here

We have done in the in other places (DS messages) and it's better than a button. Back then vsc didn't support this.
The link will also prevent a flashing message
@luabud /cc

@int19h
Copy link

int19h commented May 7, 2020

@DonJayamanne The link is broken.

@karthiknadig
Copy link
Member Author

Notification looks like this now.
image

);
export const removePythonPathCodeWorkspaceAndSettingsJson = localize(
'diagnostics.removePythonPathCodeWorkspaceAndSettingsJson',
'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it? This will only remove the "python.pythonPath" entry from your settings.json and .code-workspace file.'
'The setting "python.pythonPath" defined in your workspace settings is now deprecated. Do you want us to delete it from your .code-workspace file and settings.json? Refer to [this documentation](https://aka.ms/AA7jfor) to learn more.'
Copy link

Choose a reason for hiding this comment

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

"this documentation" should be "the documentation" or "this page".

But maybe just shorten the entire sentence to "Learn more.", and make those two words the link?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea of just having Learn more as a link. /cc @luabud

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link

Choose a reason for hiding this comment

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

Yup, looks a lot better and much shorter.

Copy link
Member

Choose a reason for hiding this comment

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

Great suggestion! LGTM

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 7, 2020

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
0.0% 0.0% Duplication

@karthiknadig karthiknadig merged commit d6c7781 into microsoft:master May 7, 2020
karthiknadig added a commit to karthiknadig/vscode-python that referenced this pull request May 8, 2020
* Show the prompt again if user clicks on more info

* Review feedback

* Use Learn more as text for the link.
karthiknadig added a commit that referenced this pull request May 8, 2020
…11700)

* Show the prompt again if user clicks on more info (#11664)

* Show the prompt again if user clicks on more info

* Review feedback

* Use Learn more as text for the link.

* Leave pipenv in a corner until the user decides to select an interpreter (#11654)

* add onSuggestion option
* Swap onActivation with onSuggestion
* Update unit tests
* Remove registration of IPipenvService
* Move didTriggerInterpreterSuggestions logic inside pipenv locator
* Fix existing unit tests
* Add new unit tests
* Replace typemoq any param with object
* Shorten the tests
* Fix warning
* Duplicate teardown

Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com>
@karthiknadig karthiknadig deleted the issue11648 branch May 12, 2020 19:05
karthiknadig added a commit that referenced this pull request May 19, 2020
* Revert vscode-extension-telemetry changes for the release (#11602) (#11656)

* Revert "Fix slashes in telemetry unit tests (#11572)"

This reverts commit 7431c9c.

* Revert "Use vscode-extension-telemetry for our exceptions & error telemetry (#11524)"

This reverts commit d5065e6.

* Remove from changelog

* Port storage fix to release branch (#11673)

* Fix storage not being used (#11649)

* Fix storage not being used

* Add disposable to storage so it won't write after shutdown

* Fix dirty title

* Hack to get tests to pass

* Another way to get run all to not interfere

* Update changelog

* Port scrolling fix to release (#11688)

* Fix scrolling (#11681)

* Fix scrolling

* Review feedback - fix scrolling on expand/collapse

* Update changelog

* Update package.json

Co-authored-by: Jim Griesmer <jimgries@microsoft.com>

* Cherry-pick pipenv changes and pythonpath prompt changes to release (#11700)

* Show the prompt again if user clicks on more info (#11664)

* Show the prompt again if user clicks on more info

* Review feedback

* Use Learn more as text for the link.

* Leave pipenv in a corner until the user decides to select an interpreter (#11654)

* add onSuggestion option
* Swap onActivation with onSuggestion
* Update unit tests
* Remove registration of IPipenvService
* Move didTriggerInterpreterSuggestions logic inside pipenv locator
* Fix existing unit tests
* Add new unit tests
* Replace typemoq any param with object
* Shorten the tests
* Fix warning
* Duplicate teardown

Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com>

* Update extension version (#11730)

* Update extension version

* Update date in changelog.

* Update change log with additional notes. (#11764)

* Cherry picks and version updates for bug fix release (#11878)

* Do not execute shebang as an interpreter until user has clicked on the codelens enclosing the shebang (#11816)

* Do not execute shebang as an interpreter until user has clicked on the codelens enclosing the shebang

* Rename

* Oops

* Update src/test/providers/shebangCodeLenseProvider.unit.test.ts

Co-authored-by: Karthik Nadig <kanadig@microsoft.com>

Co-authored-by: Karthik Nadig <kanadig@microsoft.com>

* Update version and change log for bugfix release

Co-authored-by: Kartik Raj <karraj@microsoft.com>

* Clean up news

Co-authored-by: Kim-Adeline Miguel <51720070+kimadeline@users.noreply.github.com>
Co-authored-by: Rich Chiodo <rchiodo@users.noreply.github.com>
Co-authored-by: Jim Griesmer <jimgries@microsoft.com>
Co-authored-by: Kartik Raj <karraj@microsoft.com>
@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.
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants