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

Fix update-path to work with virtualenv>=20.26.6 #32

Merged

Conversation

ATRAN2
Copy link

@ATRAN2 ATRAN2 commented Sep 30, 2024

Fix for this issue #31
Virtualenv 20.26.6 is no longer writing the VIRTUAL_ENV variable with any kind of quotation marks so we need to loosen our checks to include that
Without the quotation marks, newlines get included in the variable, so we'll need to also include a .strip()

if line.startswith(possible):
return line.split(possible[-1], 2)[1]
return line.split(possible[-1], 2)[1].strip()
Copy link
Collaborator

@asottile asottile Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line makes no sense now (and is broken if the path contained an =)

its intent is to strip quotes so that should not be happening in the unquoted version

Copy link
Collaborator

@asottile asottile Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specifically: the code that writes this uses shlex.quote -- this should probably use shlex.split to parse this -- something like

value, = shlex.split(line.removeprefix('VIRTUAL_ENV='))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah that definitely does work much better, thanks!
Switched to using shlex though I did have to use a split instead of removeprefix since that's not available yet in Python3.8

@Typpexs
Copy link

Typpexs commented Oct 3, 2024

Hey @ATRAN2, you are missing to change the regex of activation path:

activation_path_re = re.compile(                                                          
    r'^(?:set -gx |setenv |)VIRTUAL_ENV[ =][\'"](.*?)[\'"]\s*$',                           
)

is becoming :

_activation_path_re = re.compile(
    r'^(?:set -gx |setenv |)VIRTUAL_ENV[ =][\'"]?(.*?)[\'"]?\s*$',
) 

This one can accept VIRTUAL_ENV with quotes as optionnal

@ATRAN2 ATRAN2 force-pushed the fix-update-path-to-work-with-virtualenv-20.26.6 branch from 0cc8107 to 33d2c65 Compare October 7, 2024 20:43
@ATRAN2 ATRAN2 force-pushed the fix-update-path-to-work-with-virtualenv-20.26.6 branch from 33d2c65 to ed796e8 Compare October 10, 2024 18:34
@kaisen kaisen merged commit 5fdb6c8 into Yelp:main Oct 10, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants