-
-
Notifications
You must be signed in to change notification settings - Fork 757
Improved python tool support #673
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
base: master
Are you sure you want to change the base?
Conversation
themes/developer/developer.theme.sh
Outdated
| if [[ -n "$VIRTUAL_ENV" ]]; then | ||
| val_venv=" ($(echo "$VIRTUAL_ENV" | awk -F/ '{print $(NF-1)"/"$NF}'))" |
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'm wondering what happens when VIRTUAL_ENV contains a newline in the path. The above would output multiple aaa/bbb pairs. Also, what happens when VIRTUAL_ENV doesn't contain any /? It would output / + $VIRTUAL_ENV, but I'm not sure if it's the intended behavior. I'd suggest simply doing
| if [[ -n "$VIRTUAL_ENV" ]]; then | |
| val_venv=" ($(echo "$VIRTUAL_ENV" | awk -F/ '{print $(NF-1)"/"$NF}'))" | |
| if [[ $VIRTUAL_ENV =~ [^/]+/[^/]+$ ]]; then | |
| val_venv=" ($BASH_REMATCH)" |
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.
Also, what happens when
VIRTUAL_ENVdoesn't contain any/?
This hasn't been answered. Or is it ensured that it VIRTUAL_ENV contains /?
Co-authored-by: Koichi Murase <[email protected]>
|
Thank you about your review @akinomyoga I just pushed these changes to my branch |
just a small update :)