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 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 batch_prompt branch 2 times, most recently from 0cf2f82 to 2b24f15 Compare February 27, 2020 23:23
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

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

@gaborbernat
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@spetafree
Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

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

@spetafree
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@spetafree
Copy link
Contributor Author

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.

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat gaborbernat marked this pull request as ready for review March 3, 2020 08:55
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Thanks!

@gaborbernat gaborbernat merged commit dd37c7d into pypa:master Mar 3, 2020
@spetafree
Copy link
Contributor Author

@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
Copy link
Contributor

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

@spetafree
Copy link
Contributor Author

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
Copy link
Contributor

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

@spetafree
Copy link
Contributor Author

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
Copy link
Contributor

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No (env) indication visible - Windows 10
2 participants