Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
bpo-29779: new environment variable PYTHONHISTORY.#473
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
the-knights-who-say-ni commentedMar 5, 2017
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 thePSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username onbugs.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:
Thanks again to your contribution and we look forward to looking at it! |
mention-bot commentedMar 5, 2017
@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. |
I signed the CLA 😉 |
This is a nice change, and it would probably let me get rid of my $PYTHONSTARTUP setting. I wonder if |
@warsaw I added this to Misc/python.man |
@0xl3vi Thanks, that looks good. It's still not mentioned in |
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.
LGTM, with one very minor style nit.
Lib/site.py Outdated
history =os.path.join(os.path.expanduser('~'), | ||
'.python_history') | ||
history =set_history_file() | ||
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.
Minor style nit: You don't need this extra blank line.
@warsaw I updated main.c for |
The changes look great to me, but I'm going to wait just a bit before merging this.
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 |
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.
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.
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. |
@warsaw I tried rebasing against master and the build passed! @ned-deily I updated cmdline.rst and created new issuehttp://bugs.python.org/issue29779 P.S: I need to add bpo-XXX or just the issue number in bugs.python.org? |
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.
Just a couple of other thoughts on the implementation and NEWS file entry.
Lib/site.py Outdated
if history_file: | ||
return history_file | ||
return os.path.join(os.path.expanduser('~'), | ||
'.python_history') |
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.
You could also do something like:
returnos.environ.get('PYTHONHISTORY',os.path.expanduser(os.path.join('~','.python_history')))
Alsoset_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.
Misc/NEWS Outdated
@@ -10,6 +10,9 @@ What's New in Python 3.7.0 alpha 1? | |||
Core and Builtins | |||
----------------- | |||
- Issue #29779: New environment variable PYTHONHISTORY if |
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.
ChangeIssue #29779
tobpo-29779
. That's the new style of marking issue numbers.
Oh, and it would be nice to add a test for codecov. |
Thanks for addressing my comments! |
ghost commentedMar 10, 2017 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
@warsaw all done, I changed set_history_file() to gethistoryfile(). |
@0xl3vi We still need a test. |
Oh and yay for Misc/NEWS conflicts. :( |
Also notethis report which I can confirm. |
@0xl3vi At the very least, I think a test covering The bug happens when the file pointed to by |
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 commentedMar 15, 2017 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
@warsaw OK updated.
The test checks if gethistoryfile() reads the variable. what do you think? |
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.
This also needs to be documented inDoc/whatsnew/3.7.rst
(and please add "(Contributed by Your Name.)")
Thanks!
.. 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. |
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.
``~/.python_history``
@@ -713,6 +713,13 @@ conflict. | |||
.. versionadded:: 3.6 | |||
.. envvar:: PYTHONHISTORY | |||
If set to a non-empty string, you can change the location of a python_history |
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.
"a ``.python_history`` file" or just "a history file".
if not use the default ~/.python_history file. | ||
""" | ||
h = os.environ.get("PYTHONHISTORY") | ||
if h != '': |
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.
What ifos.environ.get("PYTHONHISTORY")
returnsNone
?
@@ -259,6 +259,10 @@ def test_getsitepackages(self): | |||
wanted = os.path.join('xoxo', 'lib', 'site-packages') | |||
self.assertEqual(dirs[1], wanted) | |||
def test_gethistoryfile(self): | |||
os.environ['PYTHONHISTORY'] = "xoxo" |
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.
Please useEnvironmentVarGuard
intest.support
to set env variables.
@@ -10,6 +10,9 @@ What's New in Python 3.7.0 alpha 1? | |||
Core and Builtins | |||
----------------- | |||
- bpo-29779: New environment variable PYTHONHISTORY if | |||
this is set you can change the location of a python_history file. |
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.
Please add "Patch by Your Name."
@@ -431,6 +431,10 @@ values. | |||
The integer must be a decimal number in the range [0,4294967295]. Specifying | |||
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. |
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.
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.
@@ -105,7 +105,8 @@ static const char usage_6[] = | |||
" predictable seed.\n" | |||
"PYTHONMALLOC: set the Python memory allocators and/or install debug hooks\n" | |||
" 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"; |
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.
This line is a bit long.
OK, I don't have time to do it. |
@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. |
@warsaw sure if you want. |
@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] <support@github.com>Co-authored-by: Łukasz Langa <lukasz@langa.pl>
if this variable is set the user can change the location of
a ~/.python_history file without adding hook to PYTHONSTARTUP.