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

Detect pipenv by looking for Pipfile, not pipfile #994

Merged
merged 1 commit into from
Mar 9, 2018

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:/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:/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:/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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants