Skip to content

Conversation

@wjt
Copy link

@wjt wjt commented Mar 9, 2018

On platforms with case-sensitive filesystems, like Linux, these are not equivalent. pipenv documents that the file should be called Pipfile and Pipfile.find() only finds files matching this exact case.

As a result, even if pipenv --venv in cwd would return success, it will never be run on Linux, and Code never detects the pipenv. (You can work around this with touch pipfile.) With this change, it's detected successfully. I believe there's no need to add a backwards-compatibility check for the old case, because on platforms where the old, incorrect check worked, so will the new, correct one.

On platforms with case-sensitive filesystems, like Linux, these are not equivalent. pipenv documents that the file should be called Pipfile[0] and `Pipfile.find()` only finds files matching this exact case[1]. As a result, even if `pipenv --venv` in `cwd` would return success, it will never be run on Linux, and Code never detects the pipenv. (You can work around this with `touch pipfile`.) With this change, it's detected successfully. I believe there's no need to add a backwards-compatibility check for the old case, because on platforms where the old, incorrect check worked, so will the new, correct one. [0] https://docs.pipenv.org/basics/#example-pipfile-pipfile-lock [1] https://github.com/pypa/pipfile/blob/5acb9ac7/pipfile/api.py#L76-L85
@codecov
Copy link

codecov bot commented Mar 9, 2018

Codecov Report

Merging #994 into master will increase coverage by 0.26%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #994 +/- ## ========================================== + Coverage 64.08% 64.34% +0.26%  ========================================== Files 260 260 Lines 12013 12013 Branches 2134 2134 ========================================== + Hits 7698 7730 +32  + Misses 4306 4274 -32  Partials 9 9
Impacted Files Coverage Δ
...ent/interpreter/locators/services/pipEnvService.ts 68.08% <100%> (ø) ⬆️
src/client/linters/lintingEngine.ts 90.35% <0%> (-0.88%) ⬇️
...reter/locators/services/cacheableLocatorService.ts 93.87% <0%> (+2.04%) ⬆️
src/client/common/installer/productInstaller.ts 65.95% <0%> (+4.25%) ⬆️
src/client/linters/baseLinter.ts 91.66% <0%> (+6.25%) ⬆️
src/client/common/application/applicationShell.ts 30.76% <0%> (+7.69%) ⬆️
src/client/common/logger.ts 53.33% <0%> (+20%) ⬆️
src/client/linters/errorHandlers/errorHandler.ts 100% <0%> (+22.22%) ⬆️
src/client/linters/errorHandlers/notInstalled.ts 94.44% <0%> (+61.11%) ⬆️

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 ece0479...e9b3f0e. Read the comment docs.

@brettcannon brettcannon merged commit 6323a32 into microsoft:master Mar 9, 2018
@brettcannon
Copy link
Member

Thanks for the quick fix @wjt ! As you can tell our Linux testing missed this. We will get a 2018.2.1 release out shortly with this in it.

brettcannon pushed a commit to brettcannon/vscode-python that referenced this pull request Mar 9, 2018
On platforms with case-sensitive filesystems, like Linux, these are not equivalent. pipenv documents that the file should be called Pipfile[0] and `Pipfile.find()` only finds files matching this exact case[1]. As a result, even if `pipenv --venv` in `cwd` would return success, it will never be run on Linux, and Code never detects the pipenv. (You can work around this with `touch pipfile`.) With this change, it's detected successfully. I believe there's no need to add a backwards-compatibility check for the old case, because on platforms where the old, incorrect check worked, so will the new, correct one. [0] https://docs.pipenv.org/basics/#example-pipfile-pipfile-lock [1] https://github.com/pypa/pipfile/blob/5acb9ac7/pipfile/api.py#L76-L85
DonJayamanne pushed a commit that referenced this pull request Mar 9, 2018
* Detect pipenv by looking for Pipfile, not pipfile (#994) On platforms with case-sensitive filesystems, like Linux, these are not equivalent. pipenv documents that the file should be called Pipfile[0] and `Pipfile.find()` only finds files matching this exact case[1]. As a result, even if `pipenv --venv` in `cwd` would return success, it will never be run on Linux, and Code never detects the pipenv. (You can work around this with `touch pipfile`.) With this change, it's detected successfully. I believe there's no need to add a backwards-compatibility check for the old case, because on platforms where the old, incorrect check worked, so will the new, correct one. [0] https://docs.pipenv.org/basics/#example-pipfile-pipfile-lock [1] https://github.com/pypa/pipfile/blob/5acb9ac7/pipfile/api.py#L76-L85 * Prep for 2018.2.1
@wjt wjt deleted the fix-Pipfile-case branch March 10, 2018 15:43
@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.

Labels

None yet

2 participants