Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refines how the uv virtual environment is controlled by explicitly managing environment variables for seed and exclusion flags. Key changes include:
- Updating tests (tests/test_describe.py) to set the UV_VENV_SEED environment variable.
- Modifying create_venv_with_uv in package.py to remove UV_VENV_SEED from the environment and conditionally include the "--seed" flag.
- Updating pip_install and its callers (package.py and init.py) to propagate an optional env parameter for more explicit environment management.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_describe.py | Adds monkeypatch-based environment variable setting for tests. |
| src/pickley/package.py | Adjusts virtual environment creation and pip_install to manage env variables. |
| src/pickley/init.py | Propagates environment changes for UV_EXCLUDE_NEWER via pip_install. |
Comments suppressed due to low confidence (2)
src/pickley/package.py:55
- Ensure that runez.run properly handles a None value for the 'seed' argument; consider filtering out None values from the argument list to avoid unintended behavior.
r = runez.run(uv_path, "-q", "venv", seed, "-p", self.settings.python_executable, self.folder, logger=self.logger, env=env)
src/pickley/init.py:252
- [nitpick] Confirm that passing an env parameter (which may be None) to pip_install does not alter the intended behavior; consider documenting this behavior if it is intentional.
r = venv.pip_install(pip_spec, no_deps=True, quiet=True, fatal=False, env=env)
|
|
||
| if self.use_pip: | ||
| return self._run_py_pip(*cmd, *args, fatal=fatal, passthrough=passthrough) | ||
| return self._run_py_pip(*cmd, *args, fatal=fatal, passthrough=passthrough, env=env) |
There was a problem hiding this comment.
Verify that _run_py_pip (and similarly _run_uv on line 91) can handle the additional 'env' parameter; update their signatures if needed to ensure consistency.
thatch
approved these changes
Apr 1, 2025
Member
thatch
left a comment
There was a problem hiding this comment.
Seems like a step in the right direction; I wish there was a more standard way to just get a "sanitized" environ to start with.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.