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

Suppressing .env loading message when PIPENV_QUIET is set #3457

Closed
wants to merge 5 commits into from

Conversation

thunderk
Copy link

This is a proposed fix for issue #2358 that have been closed without solution.

The fix suppresses the message "Loading .env environment variables" that is sent to stderr, when PIPENV_QUIET environment variable is set.

This is very useful for people that use pipenv run in cron commands, because writing to stderr may systematically report the cron as error.

with open(dotenv_path, 'w') as f:
pass

with mock.patch('pipenv.environments.PIPENV_DOTENV_LOCATION', dotenv_path):
Copy link
Contributor

@frostming frostming Jan 18, 2019

Choose a reason for hiding this comment

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

It's better to set PIPENV_QUIET environment variable, rather than patching the function here.

Copy link
Author

Choose a reason for hiding this comment

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

Well I wanted to to this, but the PIPENV_QUIET variable is removed at import-time :

del PIPENV_VERBOSE

Do you think patching PIPENV_VERBOSITY variable is acceptable instead ?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I meant this line :

del PIPENV_QUIET

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, setting PIPENV_VERBOSITY to -1 looks fine to me.

Copy link
Author

Choose a reason for hiding this comment

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

Ok thanks, changes done

Copy link
Author

Choose a reason for hiding this comment

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

I can squash in one commit if you prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have preference on this. Either is ok for me

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I just pushed the squashed commit

@jxltom
Copy link
Contributor

jxltom commented Jan 18, 2019

Why the CI in windows for python27 can't pass...

@thunderk
Copy link
Author

I don't know why CI fails on windows, #3426 has the same error.

It fails while creating the virtualenv, with the error:
UnicodeDecodeError: 'utf8' codec can't decode byte 0xeb in position 5: invalid continuation byte

Log is here : https://dev.azure.com/pypa/pipenv/_build/results?buildId=3852&view=logs&jobId=b63f28c9-dd61-5e61-2539-dacb59be920f&taskId=838050e5-b1f9-5e09-c266-e53e847c68d9&lineStart=75&lineEnd=76&colStart=1&colEnd=1

I'm not sure how this could be related with my changes, they do not include non-ascii characters.

@jxltom
Copy link
Contributor

jxltom commented Jan 18, 2019

Your PR looks good to me. I don't think the failing CI is related with your PR since it is a so minor change. But it still worth investigating why the CI fails...

@jxltom
Copy link
Contributor

jxltom commented Jan 18, 2019

One more thing to say is that we might still need to make the test pass first, otherwise merging this may make our CI failing all the time.

@thunderk Do you mind add a news first?

@thunderk
Copy link
Author

@jxltom Sure, I added the news, I hope it's ok

@techalchemy
Copy link
Member

Offhand, are you using an editor that respects .editorconfig and saves everything in utf8 mode? If not, you could easily have pushed a source file with the wrong encoding which would cause problems on the decode step.

@techalchemy
Copy link
Member

(We aren’t going to merge this until it’s sorted out either way, whether it’s a CI issue or a code issue, so let’s get to the bottom of it)

@thunderk
Copy link
Author

@techalchemy Yes, I use an editor aware of .editorconfig. I checked again, and the files I edited seems to be saved as utf-8. I will try to add the coding:utf-8 header in the test file (it was missing, but I can't see non utf-8 chars in my diff).

@thunderk thunderk force-pushed the quiet_env_loading branch 2 times, most recently from 054ddd2 to c3545a3 Compare January 22, 2019 07:51
@thunderk
Copy link
Author

OK, I'm stuck, I tried to go back from master and rewrite my changes with another editor (just to be sure). Also checked the modified files with some utf-8 validator, and still no luck...
I don't have a Windows installation at hand to reproduce.

@techalchemy
Copy link
Member

well, note that all tests were failing until recently, so that may be contributing... the changes seem ok to me so i'll just update your pr with the current changes and see if that works

@techalchemy
Copy link
Member

(I think the transient test failure issue is resolved and this is probably something we can merge now!)

@techalchemy techalchemy added Category: CLI Issue relates to the CLI Priority: Medium This item is medium priority and will be resolved whenever possible. Status: Awaiting Review This item is currently awaiting review. Type: Enhancement 💡 This is a feature or enhancement request. labels May 26, 2019
@ExaneServerTeam
Copy link

Any news on this item. I am also interested to get ride this notification.

@matteius
Copy link
Member

matteius commented Apr 2, 2022

This was fixed in these recent PR:
#5006
#5010

@matteius matteius closed this Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: CLI Issue relates to the CLI Priority: Medium This item is medium priority and will be resolved whenever possible. Status: Awaiting Review This item is currently awaiting review. Type: Enhancement 💡 This is a feature or enhancement request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants