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

Changes to logic for selection of best (default) version of Python interpreter #3813

Merged
merged 48 commits into from Jan 3, 2019

Conversation

DonJayamanne
Copy link

@DonJayamanne DonJayamanne commented Dec 26, 2018

For #3369

Depends on #3809
Depends on #3812

  • Add tests for configSettings.ts by injecting IWorkspaceService.
    This must be done as Multi Root (functional) tests failed.
    We need to add unit tests to cover it.

  • Add tests (Would be nice to see convertPythonVersionToSemver get some unit test attention. Otherwise, this PR looks great. I do appreciate the large amount of work it took to replace this functionality with SemVer!)

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

@DonJayamanne DonJayamanne changed the title WIP WIP - Changes to logic for selection of best (default) version of Python interpreter Dec 27, 2018
@DonJayamanne DonJayamanne changed the title WIP - Changes to logic for selection of best (default) version of Python interpreter Changes to logic for selection of best (default) version of Python interpreter Dec 31, 2018
build/ci/mocha-vsts-reporter.js Show resolved Hide resolved
src/client/common/configuration/service.ts Outdated Show resolved Hide resolved
src/client/common/platform/registry.ts Show resolved Hide resolved
src/client/interpreter/autoSelection/index.ts Outdated Show resolved Hide resolved
src/test/interpreters/autoSelection/proxy.unit.test.ts Outdated Show resolved Hide resolved
src/test/interpreters/interpreterService.unit.test.ts Outdated Show resolved Hide resolved
src/test/linters/linterCommands.unit.test.ts Show resolved Hide resolved
src/test/linters/linterManager.unit.test.ts Show resolved Hide resolved
@d3r3kk
Copy link

d3r3kk commented Jan 2, 2019

Overall I like the change. There will have to be some back and forth on what constitutes the 'best interpreter' but I think you've largely nailed it for the majority of our users.

I like the approach you took with the rule evaluator, I think that will make it very simple/flexible to sort out issues or change priority of 'best interpreters' found on a system.

Copy link

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

:shipit:

@d3r3kk
Copy link

d3r3kk commented Jan 3, 2019

Still a few comments.

@DonJayamanne DonJayamanne merged commit 6c80bbc into microsoft:master Jan 3, 2019
@DonJayamanne DonJayamanne deleted the issue3369AutoX branch May 24, 2019 20:23
@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.

None yet

3 participants