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

Default prompt of environment name in batch shell #1679

Merged
merged 11 commits into from Mar 3, 2020

Conversation

@spetafree
Copy link
Contributor

spetafree commented Feb 27, 2020

Fixes #1570, by setting a default prompt for batch (windows cmd.exe) shell based on the environment name.

In bash activation script, the check for using the default prompt is done in the bash shell script. It is more complicated to check that and get the environment folder name in bat script, and because of that I have decided to implement that check in the python code, by inheriting the replacements method and adding the check there.

I had a few failures in the local tests I tried to run, but is seems to me that these are not connected to my change - the same tests happen also on the current master.

Thanks for contributing, make sure you address all the checklists (for details on how see

development documentation)!

  • ran the linter to address style issues (tox -e fix_lint)

  • wrote descriptive pull request text

Above.

  • ensured there are test(s) validating the fix

I am actually at loss how to test this, the current tests seem not to test that issue also on other
shells where I assume it works.

  • added news fragment in docs/changelog folder
    I do not understand how should I name the fragment, I hope it is ok.

  • updated/extended the documentation
    n/a

@spetafree spetafree force-pushed the spetafree:batch_prompt branch 2 times, most recently from 0cf2f82 to 2b24f15 Feb 27, 2020
Copy link
Contributor

gaborbernat left a comment

Was thinking personally this should be a change within activate.bat 🤔

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Feb 27, 2020

I'm not saying we absolutely must do it in batch but would like to see the two options 👍and how they would differ and if it's worth doing differently here than for all other scripts.

@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Feb 27, 2020

What do you think about the change in via_template.py? Maybe it will fix all the shells at once? The prompt will just always default to '(env_name) ' instead of to empty string in python, and the shells scripts may be simplified.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Feb 27, 2020

Can't tell from top of my head at the moment. Need to have a think about it.

@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Feb 27, 2020

It may take me some time, but may try to install some of the other shells and see how it works. There is also the question of how to specify an empty prompt in command line. (maybe we will need a --no-prompt flag)
Thanks for reviewing the PR anyway, It will probably be a few days at least until I can learn enough about all these shells, I'm much more familiar with python that with shell scripts 😄

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Feb 27, 2020

No worries, take your time, I'll not be able to take a deeper look at this until next Tuesday.

@spetafree spetafree force-pushed the spetafree:batch_prompt branch from de137f0 to 324315f Feb 28, 2020
Copy link
Contributor

gaborbernat left a comment

I like where this ended up if you can write a test to validate the change that would make it merge ready 👍

@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Mar 1, 2020

I'll try to write the test tomorrow. Do you think that adding another command to print the prompt environment variable in _get_test_lines and assert it in assert_output in test_batch.py is the correct way?

@spetafree spetafree closed this Mar 1, 2020
@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Mar 1, 2020

Any chance you'll consider re-opening it, I was not able to work on it the last couple of days? Tomorrow I'll hopefully be able to work on it again.

@gaborbernat gaborbernat reopened this Mar 1, 2020
@spetafree spetafree force-pushed the spetafree:batch_prompt branch from d75c83e to dddf7cd Mar 2, 2020
@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Mar 2, 2020

I'll explain the changes:

  1. I had to change the handling of the variables in the batch script to support the non ascii characters. The following seems to work:

if NOT DEFINED ENV_PROMPT (

  1. The folder name in tests: e-$ èрт🚒♞中片-j contains a single $ character which is an escape character in batch prompts, and causes incorrect rendering. I have changed it to $$. I hope it's ok with all other shells, and doesn't drop some edge case that was checked with single $.

  2. I have added testing with and without --prompt to all shells.

  3. I have expanded the test for batch activation test to check for correct prompt. I have not added the same test to other shells. I can do it for bash, which I am somewhat familiar with, but I do not know how to test this for other shells.

p.s. @gaborbernat, This is my second ever attempt at a public open source pull request, and I would like to thank you very much for your handling of it 😄. It is a much nicer treatment than my last attempt...

@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Mar 2, 2020

I see I missed something. Will fix in a few hours.

spetafree added 2 commits Mar 2, 2020
@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Mar 2, 2020

I have no idea why to the zipapp test fails, and how is this related to my PR.

Any help will be appreciated, I will continue investigating. But currently I can't reproduce this on my windows machine.

gaborbernat added 3 commits Mar 3, 2020
Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat gaborbernat force-pushed the spetafree:batch_prompt branch from 21da51b to 5fdf31a Mar 3, 2020
Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat gaborbernat marked this pull request as ready for review Mar 3, 2020
Copy link
Contributor

gaborbernat left a comment

Thanks!

@gaborbernat gaborbernat merged commit dd37c7d into pypa:master Mar 3, 2020
34 of 36 checks passed
34 of 36 checks passed
pypa.virtualenv Build #pypa.virtualenv_20200303.01 failed
Details
pypa.virtualenv (pypy macOs) pypy macOs was canceled
Details
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
pypa.virtualenv (dev linux) dev linux succeeded
Details
pypa.virtualenv (docs linux) docs linux succeeded
Details
pypa.virtualenv (docs windows) docs windows succeeded
Details
pypa.virtualenv (fix_lint linux) fix_lint linux succeeded
Details
pypa.virtualenv (fix_lint windows) fix_lint windows succeeded
Details
pypa.virtualenv (homebrew_py2 macOs) homebrew_py2 macOs succeeded
Details
pypa.virtualenv (homebrew_py3 macOs) homebrew_py3 macOs succeeded
Details
pypa.virtualenv (py27 linux) py27 linux succeeded
Details
pypa.virtualenv (py27 macOs) py27 macOs succeeded
Details
pypa.virtualenv (py27 windows) py27 windows succeeded
Details
pypa.virtualenv (py35 linux) py35 linux succeeded
Details
pypa.virtualenv (py35 macOs) py35 macOs succeeded
Details
pypa.virtualenv (py35 vs2017-win2016) py35 vs2017-win2016 succeeded
Details
pypa.virtualenv (py36 linux) py36 linux succeeded
Details
pypa.virtualenv (py36 macOs) py36 macOs succeeded
Details
pypa.virtualenv (py36 vs2017-win2016) py36 vs2017-win2016 succeeded
Details
pypa.virtualenv (py37 linux) py37 linux succeeded
Details
pypa.virtualenv (py37 macOs) py37 macOs succeeded
Details
pypa.virtualenv (py37 vs2017-win2016) py37 vs2017-win2016 succeeded
Details
pypa.virtualenv (py38 linux) py38 linux succeeded
Details
pypa.virtualenv (py38 macOs) py38 macOs succeeded
Details
pypa.virtualenv (py38 vs2017-win2016) py38 vs2017-win2016 succeeded
Details
pypa.virtualenv (pypy linux) pypy linux succeeded
Details
pypa.virtualenv (pypy windows) pypy windows succeeded
Details
pypa.virtualenv (pypy3 linux) pypy3 linux succeeded
Details
pypa.virtualenv (pypy3 macOs) pypy3 macOs succeeded
Details
pypa.virtualenv (pypy3 windows) pypy3 windows succeeded
Details
pypa.virtualenv (readme linux) readme linux succeeded
Details
pypa.virtualenv (readme windows) readme windows succeeded
Details
pypa.virtualenv (upgrade linux) upgrade linux succeeded
Details
pypa.virtualenv (upgrade windows) upgrade windows succeeded
Details
pypa.virtualenv (zipapp linux) zipapp linux succeeded
Details
pypa.virtualenv (zipapp windows) zipapp windows succeeded
Details
@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Mar 3, 2020

@gaborbernat Thanks a lot 😄.

But, there is no test for the prompt now, why did you decide to approve?

Also the parameteriztion in conftest.py change has no effect now as far as I understand.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Mar 3, 2020

Why would it not have any effect, I think this is alright 🤔

@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Mar 3, 2020

Because no test looks at the prompt now as far as I understand.

The only test that actually checked the prompt was in this commit:

dddf7cd

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Mar 3, 2020

That's not true, the tests already looked at it, just did not run the test suite Without prompt flag.

@spetafree

This comment has been minimized.

Copy link
Contributor Author

spetafree commented Mar 3, 2020

Ok, then I couldn't understand the tests - I didn't find a test that actually reads the prompt. I'll look again. Thanks anyway.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

gaborbernat commented Mar 4, 2020

Hello, a fix for this issue has been released via virtualenv 20.0.8; see https://pypi.org/project/virtualenv/20.0.8/ (https://virtualenv.pypa.io/en/latest/changelog.html#v20-0-8-2020-03-04). Please give a try and report back if your issue has not been addressed; if not, please comment here, and we'll reopen the ticket. We want to apologize for the inconvenience this has caused you and say thanks for having patience while we resolve the unexpected bugs with this new major release.
thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.