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

Modified Select interpreter command & add a reset interpreter command #10373

Merged
merged 4 commits into from
Mar 2, 2020

Conversation

karrtikr
Copy link

@karrtikr karrtikr commented Mar 1, 2020

I recommend to hide whitespace changes before reviewing

  • Modified Select interpreter command to support setting interpreter at workspace level
  • Added a reset interpreter command in a similar way

For #10372 #10374

  • 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 changed the base branch from master to pythonPath March 1, 2020 00:45
@karrtikr karrtikr changed the title Modified Select interpreter command to support setting interpreter at workspace level Modified Select interpreter command add a reset interpreter command Mar 1, 2020
@karrtikr karrtikr changed the title Modified Select interpreter command add a reset interpreter command Modified Select interpreter command & add a reset interpreter command Mar 1, 2020
@codecov-io
Copy link

codecov-io commented Mar 1, 2020

Codecov Report

Merging #10373 into pythonPath will increase coverage by <.01%.
The diff coverage is 76.92%.

Impacted file tree graph

@@              Coverage Diff               @@
##           pythonPath   #10373      +/-   ##
==============================================
+ Coverage       60.83%   60.83%   +<.01%     
==============================================
  Files             570      570              
  Lines           30746    30764      +18     
  Branches         4375     4379       +4     
==============================================
+ Hits            18704    18715      +11     
- Misses          11089    11094       +5     
- Partials          953      955       +2
Impacted Files Coverage Δ
src/client/interpreter/configuration/types.ts 100% <ø> (ø) ⬆️
...guration/services/workspaceFolderUpdaterService.ts 25% <0%> (ø) ⬆️
...ter/configuration/services/globalUpdaterService.ts 12.5% <0%> (ø) ⬆️
...erpreter/configuration/pythonPathUpdaterService.ts 23.07% <0%> (ø) ⬆️
src/client/common/constants.ts 100% <100%> (ø) ⬆️
src/client/common/utils/localize.ts 95.09% <100%> (+0.01%) ⬆️
...t/interpreter/configuration/interpreterSelector.ts 70.58% <94.73%> (+5.37%) ⬆️
src/datascience-ui/react-common/arePathsSame.ts 75% <0%> (-12.5%) ⬇️
src/client/common/utils/platform.ts 64.7% <0%> (-11.77%) ⬇️
src/client/linters/pydocstyle.ts 86.66% <0%> (-2.23%) ⬇️
... and 2 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 a12c6bf...76ec498. Read the comment docs.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Mostly LGTM.

I do have a question about whether the workspace quickpick part should be part of this PR or not. It seems fine here, but if there is a separate issue for it then I'd expect a separate PR.

I also recommend renaming InterpreterSelector.getWorkspaceToSetPythonPath() to getConfigTarget() and make it handle the global case.

@@ -0,0 +1 @@
Modified `Select interpreter` command to support setting interpreter at workspace level.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this part of this PR? Each PR should cover one change, so shouldn't it be in a separate PR? If it really is integral to the reset interpreter command then I don't see why it should have a separate issue number.

Copy link
Author

@karrtikr karrtikr Mar 2, 2020

Choose a reason for hiding this comment

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

Actually I had to refactor tests for Modified Select interpreter command to support setting interpreter at workspace level while writing tests for Added a reset interpreter command in a similar way.

I can move Added a reset interpreter command in a similar way it into a separate PR but it would still be based on Modified Select interpreter command to support setting interpreter at workspace level PR, so I figured maybe not. Should I do it?

Copy link
Member

Choose a reason for hiding this comment

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

There are several advantages to keeping each PR discrete. Some of those advantages:

  • easier to understand
  • easier to identify what changed in the git history
  • easier to revert

At the least I wanted to understand why one PR was addressing 2 distinct issues, before I approved the PR.

Copy link
Author

@karrtikr karrtikr Mar 2, 2020

Choose a reason for hiding this comment

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

I understand. I should've atleast left a comment explaining the reason I did that. Thanks for the feedback!

@ericsnowcurrently ericsnowcurrently removed the request for review from karthiknadig March 2, 2020 19:51
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM (assuming we sort out the question of one PR for multiple issues)

Copy link
Member

@karthiknadig karthiknadig left a comment

Choose a reason for hiding this comment

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

:shipit:

Co-Authored-By: Karthik Nadig <kanadig@microsoft.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 2, 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
No Duplication information No Duplication information

@karrtikr karrtikr merged commit c8d9e49 into microsoft:pythonPath Mar 2, 2020
@karrtikr karrtikr deleted the setInterpreter branch March 2, 2020 20:49
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
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.

4 participants