allow setting HistoryManager.hist_file with config #940

Merged
merged 2 commits into from Oct 30, 2011

Conversation

Projects
None yet
2 participants
@minrk
Member

minrk commented Oct 29, 2011

fixes trait init in HistoryAccessor, which prevented setting hist_file via config from having any effect.

Also remove dependency on having a shell object in HistoryAccessor.__init__

Associated test included.

as described in #882

allow setting HistoryManager.hist_file with config
fixes improper init in HistoryAccessor, preventing setting `hist_file` via config from having any effect.

Also remove dependency on having a shell object in `HistoryAccessor.__init__`

Associated test included.
IPython/core/tests/test_history.py
+
+def test_hist_file_config():
+ cfg = Config()
+ cfg.HistoryManager.hist_file = tempfile.mktemp()

This comment has been minimized.

@fperez

fperez Oct 29, 2011

Member

mktemp is deprecated. For this particular test, I suggest instead creating the temp file with NamedTemporaryFile:

tfile = tempfile.NamedTemporaryFile(delete=False)
cfg.HistoryManager.hist_file = tfile.name
...

try:
  nt.assert...
finally:
  os.unlink(tfile.name)

That's the right pattern for these guys. In Pyhton 3 they've been turned into context managers, which is really what you want.

@fperez

fperez Oct 29, 2011

Member

mktemp is deprecated. For this particular test, I suggest instead creating the temp file with NamedTemporaryFile:

tfile = tempfile.NamedTemporaryFile(delete=False)
cfg.HistoryManager.hist_file = tfile.name
...

try:
  nt.assert...
finally:
  os.unlink(tfile.name)

That's the right pattern for these guys. In Pyhton 3 they've been turned into context managers, which is really what you want.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 29, 2011

Member

Other than the above fixes to the test, the code looks good to me. I'll merge it once this small stuff is fixed; thanks!

Member

fperez commented Oct 29, 2011

Other than the above fixes to the test, the code looks good to me. I'll merge it once this small stuff is fixed; thanks!

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 30, 2011

Member

suggested changes made and pushed

Member

minrk commented Oct 30, 2011

suggested changes made and pushed

fperez added a commit that referenced this pull request Oct 30, 2011

Merge pull request #940 from minrk/hist_file_config
Allow setting HistoryManager.hist_file with config.  This fixes trait init in HistoryAccessor, which prevented setting hist_file via config from having any effect.

@fperez fperez merged commit 602e3c3 into ipython:master Oct 30, 2011

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #940 from minrk/hist_file_config
Allow setting HistoryManager.hist_file with config.  This fixes trait init in HistoryAccessor, which prevented setting hist_file via config from having any effect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment