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.

Sign up for GitHub

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

Jump to bottom

Support pyenv and direnv #854

Merged
merged 46 commits into from
Feb 22, 2018
Merged

Support pyenv and direnv #854

merged 46 commits into from
Feb 22, 2018

Conversation

MikhailArkhipov
Copy link

@MikhailArkhipov MikhailArkhipov commented Feb 21, 2018 •
edited
Loading

Fixes #36

  • automatic search for .direnv under the workspace

Fixes #847

  • search in ~/.pyenv for virtual envs

Mikhail Arkhipov added 30 commits February 16, 2018 10:50
Fix pylint search
6546892
Handle quote escapes in strings
76af122
Escapes in strings
5d4d022
CR feedback
29edac2
Missing pip
0492aab
Test
d0a449f
Tests
55197c3
Tests
f6a0123
Mac python path
d07d3ef
Tests
2b0cc92
Tests
3867ec2
Test
85fc4ef
"Go To Python object" doesn't work
00887a4
Proper detection and type population in virtual env
f89dd96
Test fixes
32394e2
Simplify venv check
2e9c039
Remove duplicates
ec563c7
Test
2ad4475
Discover pylintrc better + tests
1ee0be2
Merge branch 'master' into 804
44665b9
Merge branch 'master' of https://github.com/Microsoft/vscode-python i…
29d3925 
…nto 804
Undo change
04c5733
CR feedback
37328a5
Set interprereter before checking install
55ff4e5
Merge master
9a0cfa8
Merge master
023af49
Fix typo and path compare on Windows
436e5a9
Rename method
a53d815
#815 - 'F' in flake8 means warning
7f3e4fa
730 - same folder temp
da034f4
@brettcannon
Copy link
Member

According to the docs, PYENV_ROOT can override the default ~/.pyenv setting.

Would it also be worth following shim resolution to automatically select a Python interpreter like we do for virtual environments created in the directory?

@MikhailArkhipov
Copy link
Author

MikhailArkhipov commented Feb 22, 2018 •
edited
Loading

Sure we can handle PYENV_ROOT. Yes, we can auto-select if it is a single instance. Now, it user selected different one in the previous session we should not be switching away, right?

@codecov
Copy link

codecov bot commented Feb 22, 2018 •
edited
Loading

Codecov Report

Merging #854 into master will increase coverage by 0.81%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #854      +/-   ##
==========================================
+ Coverage   63.52%   64.33%   +0.81%     
==========================================
  Files         257      257              
  Lines       11850    11859       +9     
  Branches     2111     2112       +1     
==========================================
+ Hits         7528     7630     +102     
+ Misses       4314     4220      -94     
- Partials        8        9       +1
Impacted Files Coverage Δ
...er/locators/services/workspaceVirtualEnvService.ts 100% <100%> (+3.84%) ⬆️
...reter/locators/services/globalVirtualEnvService.ts 100% <100%> (+5.88%) ⬆️
src/client/common/installer/productInstaller.ts 66.84% <0%> (+4.34%) ⬆️
src/client/linters/baseLinter.ts 91.66% <0%> (+6.25%) ⬆️
src/client/common/application/applicationShell.ts 30.76% <0%> (+7.69%) ⬆️
...rpreter/locators/services/baseVirtualEnvService.ts 89.58% <0%> (+14.58%) ⬆️
src/client/common/logger.ts 73.33% <0%> (+20%) ⬆️
src/client/linters/errorHandlers/errorHandler.ts 100% <0%> (+22.22%) ⬆️
src/client/common/installer/installer.ts 64.73% <0%> (+30.05%) ⬆️
... and 1 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 b70e8a2...005875f. Read the comment docs.

DonJayamanne
DonJayamanne requested changes Feb 22, 2018
View reviewed changes
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.

Please use ICurrentProcess instead of process

@MikhailArkhipov
Copy link
Author

It is more complicated, needs separate module installer and not pip

@DonJayamanne
Copy link

I guess we can implement that later as an enhancement.

@MikhailArkhipov
Copy link
Author

Separated pipenv work for #404, reopening

DonJayamanne
DonJayamanne approved these changes Feb 22, 2018
View reviewed changes
src/test/format/extension.format.test.ts
@@ -109,7 +109,7 @@ suite('Formatting', () => {
const formattedFile = path.join(targetDir, resultsName);

if (!fs.pathExistsSync(targetDir)) {
fs.mkdirSync(targetDir);
fs.mkdirpSync(targetDir);
Copy link

Choose a reason for hiding this comment

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

lol, I couldn't be bothered to identify the problem, I just created the folder with a dummy .ts file in the formatting directory.
You might want to merge master

Copy link
Author

Choose a reason for hiding this comment

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

Okaaaay :-)

Copy link

Choose a reason for hiding this comment

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

My solution was to ensure the directory always exists. I didn't check what code was looking for it.

@MikhailArkhipov MikhailArkhipov merged commit 1cf3d15 into microsoft:master Feb 22, 2018
@MikhailArkhipov MikhailArkhipov deleted the pyenv branch February 27, 2018 17:27
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DonJayamanne DonJayamanne DonJayamanne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support pyenv "Select interpreter" does not support direnv environments
3 participants
@MikhailArkhipov @brettcannon @DonJayamanne