bpo-29779: new environment variable PYTHONHISTORY.#473

ghost · 2017-03-05T06:22:27Z

if this variable is set the user can change the location of
a ~/.python_history file without adding hook to PYTHONSTARTUP.

the-knights-who-say-ni · 2017-03-05T06:22:28Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement. The "bugs.python.org username " requested by the form is the "Login name" field under "Your Details".
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

mention-bot · 2017-03-05T06:22:29Z

@0xl3vi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brettcannon, @tiran, @vsajip, @doko42 and @freddrake to be potential reviewers.

ghost · 2017-03-05T06:33:11Z

I signed the CLA 😉

warsaw · 2017-03-05T22:40:15Z

This is a nice change, and it would probably let me get rid of my $PYTHONSTARTUP setting. I wonder if python3 -h should mention this new envar?

ghost · 2017-03-08T20:53:29Z

@warsaw I added this to Misc/python.man

warsaw · 2017-03-08T21:25:57Z

@0xl3vi Thanks, that looks good. It's still not mentioned in python3 -h but I'm not entirely sure it needs to. There are other PYTHON* environment variables not mentioned in the --help output.

warsaw

LGTM, with one very minor style nit.

warsaw · 2017-03-08T21:26:40Z

Lib/site.py

Minor style nit: You don't need this extra blank line.

ghost · 2017-03-09T12:38:44Z

@warsaw I updated main.c for python -h,
blank line fixed.

warsaw · 2017-03-09T20:49:00Z

The changes look great to me, but I'm going to wait just a bit before merging this.

I'd like to see if any other core dev has an opinion about the change
The AppVeyor test is failing, although I suspect that is unrelated to your change

I don't have a Windows environment to test that on. Maybe @zooba can weigh in on that. Alternatively, try rebasing against master if it's been fixed there.

If there are no other follow ups by this weekend, I'll merge your change.

Can you please also add a Misc/NEWS entry (knowing that our workflow for that kind of sucks and we might get conflicts)?

ned-deily

Environment variables are also documented in the Python Setup and Usage document (https://docs.python.org/3/using/cmdline.html#environment-variables) which is generated from Doc/using/cmdline.rst.

ned-deily · 2017-03-09T22:02:18Z

Also there should be an issue opened on bugs.python.org to document this and the issue number added to the title of this pull request.

ghost · 2017-03-10T11:00:43Z

@warsaw I tried rebasing against master and the build passed!
and Misc/NEWS updated.

@ned-deily I updated cmdline.rst and created new issue http://bugs.python.org/issue29779

P.S: I need to add bpo-XXX or just the issue number in bugs.python.org?

warsaw

Just a couple of other thoughts on the implementation and NEWS file entry.

warsaw · 2017-03-10T19:01:56Z

Lib/site.py

You could also do something like:
returnos.environ.get( 'PYTHONHISTORY', os.path.expanduser(os.path.join('~', '.python_history')))
Also set_history_file() is a little bit of a misnomer since it's not actually setting it, but calculating it instead. OTOH, I bet the more compact form could just be inlined below.

warsaw · 2017-03-10T19:02:28Z

Misc/NEWS

Change Issue #29779 to bpo-29779. That's the new style of marking issue numbers.

warsaw · 2017-03-10T19:03:49Z

Oh, and it would be nice to add a test for codecov.

ned-deily · 2017-03-10T20:45:07Z

Thanks for addressing my comments!

ghost · 2017-03-10T21:59:31Z

@warsaw all done, I changed set_history_file() to gethistoryfile().
and added test.

warsaw · 2017-03-14T21:42:10Z

@0xl3vi We still need a test.

warsaw · 2017-03-14T21:43:34Z

Oh and yay for Misc/NEWS conflicts. :(

warsaw · 2017-03-14T22:05:55Z

Also note this report which I can confirm.

ghost · 2017-03-15T11:59:59Z

@warsaw sorry for the dealy.
what tests we need add?

regarding this
If the PYTHONHISTORY is empty it will fall back to .python_history.
there is no error on my build

warsaw · 2017-03-15T14:13:14Z

@0xl3vi At the very least, I think a test covering gethistoryfile() would be useful. If you can mock out enough of the readline stuff (or other safeguards so as not to interfere with the environment of a person running the tests), then adding some coverage for enablerlcompleter() would be nice.

The bug happens when the file pointed to by PYTHONHISTORY does not exist.

this patch adds new environment variable PYTHONHISTORY, with this you can change the location of a python_history file, without using readline hook in PYTHONSTARTUP. In case PYTHONHISTORY will be empty or not set, it wil fall back to the default history file.

ghost · 2017-03-15T15:11:37Z

@warsaw OK updated.

In case PYTHONHISTORY will be empty or not set,
it wil fall back to the default history file.
if the file pointed by PYTHONHISTORY does not exist readline will create one.

The test checks if gethistoryfile() reads the variable.
Bash will expand '~' so we dont need to to test the path.

what do you think?

berkerpeksag

This also needs to be documented in Doc/whatsnew/3.7.rst (and please add "(Contributed by Your Name.)")

Thanks!

berkerpeksag · 2017-03-22T08:33:41Z

Doc/using/cmdline.rst

+.. envvar:: PYTHONHISTORY
+
+ If set to a non-empty string, you can change the location of a python_history
+ file, by default it will be in ~/.python_history.


``~/.python_history``

berkerpeksag · 2017-03-22T08:34:45Z

Doc/using/cmdline.rst


+.. envvar:: PYTHONHISTORY
+
+ If set to a non-empty string, you can change the location of a python_history


"a ``.python_history`` file" or just "a history file".

berkerpeksag · 2017-03-22T08:36:41Z

Lib/site.py

+ if not use the default ~/.python_history file.
+ """
+ h = os.environ.get("PYTHONHISTORY")
+ if h != '':


What if os.environ.get("PYTHONHISTORY") returns None?

berkerpeksag · 2017-03-22T08:39:21Z

Lib/test/test_site.py

 self.assertEqual(dirs[1], wanted)

+ def test_gethistoryfile(self):
+ os.environ['PYTHONHISTORY'] = "xoxo"


Please use EnvironmentVarGuard in test.support to set env variables.

berkerpeksag · 2017-03-22T08:40:29Z

Misc/NEWS

 -----------------

+- bpo-29779: New environment variable PYTHONHISTORY if
+ this is set you can change the location of a python_history file.


Please add "Patch by Your Name."

berkerpeksag · 2017-03-22T08:47:18Z

Misc/python.man

 the value 0 will disable hash randomization.
+.IP PYTHONHISTORY
+If this is set you can change the location of a
+python_history file, by default it will be ~/.python_history.


File names should be italic in man pages so \fI~/.python_history\fI or
python_history file, by default it will be .IR ~/.python_history .
You may need to escape ~ or / characters.

berkerpeksag · 2017-03-22T08:48:15Z

Modules/main.c

 " on Python memory allocators. Use PYTHONMALLOC=debug to install debug\n"
-" hooks.\n"
+" hooks.\n"
+"PYTHONHISTORY: If this is set, you can change the location of a python_history file.\n"


This line is a bit long.

ghost · 2017-03-23T09:10:13Z

OK, I don't have time to do it.
If someone else want to implement this please do so.

warsaw · 2017-03-27T15:46:22Z

@0xl3vi Hi. I'm sorry this has taken so long. Are you saying in your comment that you don't have time to finish this branch? If so, that's okay and we greatly appreciate your contribution so far. I'm willing to finish this branch for you if you'd like.

ghost · 2017-03-28T21:31:32Z

@warsaw sure if you want.
thanks 👍

ZackerySpytz · 2019-05-15T08:30:35Z

@warsaw @berkerpeksag I have created a new PR for this feature: GH-13208. It addresses all of @berkerpeksag's comments.

Bumps [sentry-sdk](https://github.com/getsentry/sentry-python) from 1.1.0 to 1.3.1. - [Release notes](https://github.com/getsentry/sentry-python/releases) - [Changelog](https://github.com/getsentry/sentry-python/blob/master/CHANGELOG.md) - [Commits](getsentry/sentry-python@1.1.0...1.3.1) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Łukasz Langa <[email protected]>

the-knights-who-say-ni added the CLA not signed label Mar 5, 2017

methane removed the CLA not signed label Mar 7, 2017

the-knights-who-say-ni added the CLA signed label Mar 7, 2017

warsaw approved these changes Mar 8, 2017
View reviewed changes

Lib/site.py Outdated
Copy link
Member
warsawMar 8, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style nit: You don't need this extra blank line.

ned-deily requested changes Mar 9, 2017
View reviewed changes

ghost changed the title ~~New environment variable PYTHONHISTORY.~~bpo-29779: new environment variable PYTHONHISTORY.Mar 10, 2017

warsaw requested changes Mar 10, 2017
View reviewed changes

ned-deily approved these changes Mar 10, 2017
View reviewed changes

berkerpeksag requested changes Mar 22, 2017
View reviewed changes

ghost closed this Mar 23, 2017

0xl3vimannequin mentioned this pull request Apr 10, 2022
New environment variable PYTHON_HISTORY #73965
Closed

This pull request was closed.


		.. envvar:: PYTHONHISTORY

		If set to a non-empty string, you can change the location of a python_history

Uh oh!

bpo-29779: new environment variable PYTHONHISTORY.#473

bpo-29779: new environment variable PYTHONHISTORY. #473

Uh oh!

Conversation

ghost commented Mar 5, 2017

Uh oh!

the-knights-who-say-ni commented Mar 5, 2017

Uh oh!

mention-bot commented Mar 5, 2017

Uh oh!

ghost commented Mar 5, 2017

Uh oh!

warsaw commented Mar 5, 2017

Uh oh!

ghost commented Mar 8, 2017

Uh oh!

warsaw commented Mar 8, 2017

Uh oh!

warsaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 9, 2017

Uh oh!

warsaw commented Mar 9, 2017

Uh oh!

ned-deily left a comment

Choose a reason for hiding this comment

Uh oh!

ned-deily commented Mar 9, 2017

Uh oh!

ghost commented Mar 10, 2017

Uh oh!

warsaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

warsaw commented Mar 10, 2017

Uh oh!

ned-deily commented Mar 10, 2017

Uh oh!

ghost commented Mar 10, 2017• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

warsaw commented Mar 14, 2017

Uh oh!

warsaw commented Mar 14, 2017

Uh oh!

warsaw commented Mar 14, 2017

Uh oh!

ghost commented Mar 15, 2017

Uh oh!

warsaw commented Mar 15, 2017

Uh oh!

ghost commented Mar 15, 2017• edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 23, 2017

ghost commented Mar 10, 2017•
edited by ghost
Loading

ghost commented Mar 15, 2017•
edited by ghost
Loading