Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Raise default PIPENV_MAX_DEPTH to 10 #6214

Merged
merged 3 commits into from
Aug 23, 2024
Merged

Raise default PIPENV_MAX_DEPTH to 10 #6214

merged 3 commits into from
Aug 23, 2024

Conversation

snickell
Copy link
Contributor

@snickell snickell commented Jul 30, 2024

Currently pipenv will only look up 2 directory levels (default is now set to 3, but behaviorally this includes the root directory!) for a Pipfile. That means if you are in /src/myfeature/test and you use pipenv, it will not find the virtualenv for /Pipfile, but assume/create a new one.

Why make the change?

  • Principle of least surprise: pipenv run should work the same run anywhere in a repo. Even pipenv itself's repo has sub-directories deeper than 2 levels. Figuring out what broke and why takes debugging time and causes confusion.
  • Works out of the box: makes pipenv work in deeper/larger repos without having to instruct developers to set new shell variables.
  • Matches behavior of, e.g. bundle exec from ruby (I believe bundle exec actually has an infinite depth, but its closer!)

Why not make the change?

  • Performance: in theory, one might worry that adding 7 more stat operations could slow things down however:
    • Only slows down by 0.3 ms on my system (benchmarked find_pipfile(max_depth=10))
    • And even that, only on the edge case where you pipenv install on a repo that does not yet have a Pipfile: i.e. it only slows down "Pipfile not found" behavior, and it avoids erroneous "not founds".

Alternatives considered

  • Maybe there shouldn't be a max depth: filesystem stats are incredibly fast. I think this would maintain the principle of least surprise, but could result in pathological cases. Perhaps having a limit is best, but it should be high enough to be exceptionally rare to hit.
  • 10 was chosen mostly because its 10, but also because whereas I frequently see github repos with depths beyond 3 (src/subproject/test and we're already there, as . is included in the max depth count), I can't remember seeing a repo that was 10 levels deep. Its still out there, somebody will get broken, but the long tail is now much much rarer (compare to 3 levels, which I bet most popular repos exceed).

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

@snickell snickell marked this pull request as draft July 30, 2024 21:05
@snickell snickell marked this pull request as ready for review July 30, 2024 21:20
@oz123 oz123 requested review from oz123 and matteius August 19, 2024 19:36
@oz123
Copy link
Contributor

oz123 commented Aug 19, 2024

@snickell thank you for creating this PR. I agree with your argumentation.
I don't consider this a breaking change, but I still want to hear @matteius stand on this.

Can you please fix the linting issue ?

@oz123 oz123 merged commit 10f5c3d into pypa:main Aug 23, 2024
19 checks passed
@snickell snickell deleted the patch-1 branch August 24, 2024 19:29
@snickell
Copy link
Contributor Author

Wow, you guys are quick, really appreciated the responsiveness, not easy to do with open source 🙏

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants