fix: Patch get_interpreter to handle missing cache and app_data #2974
Conversation
* Patch `get_interpreter` in `src/virtualenv/discovery/builtin.py` to handle missing `cache` and `app_data` arguments. * Add a new test case to `tests/unit/discovery/test_discovery.py` to ensure `get_interpreter` works without `cache` and `app_data` arguments. Signed-off-by: Emre Şafak <3928300+esafak@users.noreply.github.com>
for more information, see https://pre-commit.ci
| :param env: the environment to use | ||
| :return: the interpreter if found, otherwise None | ||
| """ | ||
| if cache is None: |
There was a problem hiding this comment.
Doesn't this make the extraction of the discovery not possible?
There was a problem hiding this comment.
Do you have a suggestion? We can sleep on it.
There was a problem hiding this comment.
Not on top of my mind, but perhaps is indicative that we cannot extract discovery?
There was a problem hiding this comment.
We could keep this for a breaking change requiring the cache. Maybe other users will have ideas.
There was a problem hiding this comment.
But if you don't have a good use case why introduce the breaking changes? We could revert this set of changes into a single commit and if anyone has good ideas to solve it in the future they can always revert our revert and we can cross that bridge at that point?
Sorry for bringing this up only now my bad I definitely should have realized earlier that this is a lot more breaking than I initially expected.
There was a problem hiding this comment.
The use case was extraction of discovery in order to reduce the package's footprint, was it not?
There was a problem hiding this comment.
The use case was extraction of discovery in order to reduce the package's footprint, was it not?
Not entirely, the goal was to allow other people use it without deepending on the heavier size of the virtualenv project. If we're not going to achieve that goal today the benefit of this separation significantly decreases too. Not to mention, originally was requested pipx, whose development now stalled due to poeple moving to uv tool.
2 participants
get_interpreterinsrc/virtualenv/discovery/builtin.pyto handle missingcacheandapp_dataarguments.tests/unit/discovery/test_discovery.pyto ensureget_interpreterworks withoutcacheandapp_dataarguments.Fixes #2972
tox -e fix)docs/changelogfolder