-
Notifications
You must be signed in to change notification settings - Fork 79
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
misc: use .venv by default instead of venv #3629
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3629 +/- ##
=======================================
Coverage 90.15% 90.15%
=======================================
Files 465 465
Lines 58733 58733
Branches 5629 5629
=======================================
Hits 52949 52949
Misses 4345 4345
Partials 1439 1439 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that this may break some dev phonies in the Makefile for existing users with a venv/
already generated, as it will create a new .venv
directory when they are run, but not install the VENV_EXTRAS
into them unless make venv
is run first.
This is not necessarily a big problem as they can just run make .venv/ && rm -rf venv
, but might confuse some people?
@@ -5,7 +5,7 @@ MAKEFLAGS += --no-builtin-variables | |||
COVERAGE_FILE ?= .coverage | |||
|
|||
# allow overriding the name of the venv directory | |||
VENV_DIR ?= venv | |||
VENV_DIR ?= .venv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The venv
make target on line 32 should also be renamed to .venv
, as that is now what uv sync
will create
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it locally and make venv
still creates a .venv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly, that's the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so do we want this suggestion?
We can't account for everything, this is what I did locally as well. |
I didn't want to have this discussion before we merged uv, but it definitely feels like it really prefers
.venv
, and makes lots of things easier if this name is used. How do we feel about using this name by default? It can still easily be overridden locally, but will change to .venv by default for everyone.