allow disabling SQLite history #2278
Conversation
* adds HistoryAccessor.disabled configurable for turning off the SQLite history * also provides connection_options configurable dict for relaying kwargs to sqlite3.connect. This is a just-in-case measure, but I think valuable if people have a reason to adjust the SQLite connection flags. Mainly for use in situations where threading or filesystem issues can cause problems for sqlite, e.g. embedding in django. closes ipython#2276
| """return an empty list in the absence of sqlite""" | ||
| if sqlite3 is None: | ||
| if sqlite3 is None or self.disabled: |
There was a problem hiding this comment.
I see that here you are using sqlite but below you still test against sqlite3. Are both defined? Are the right ones used in the right places?
There was a problem hiding this comment.
Nope, just a typo and already fixed.
There was a problem hiding this comment.
OK I would say this is merge ready. Great work!
On Thu, Aug 9, 2012 at 4:03 PM, Min RK notifications@github.com wrote:
In IPython/core/history.py:
"""return an empty list in the absence of sqlite"""
- if sqlite3 is None:
- if sqlite3 is None or self.disabled:
Nope, just a typo and already fixed.
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2278/files#r1347855.
Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com
|
Test results for commit b4ee0c2 merged into master (75c66c8)
Not available for testing: |
|
Other than the one comment above, the code looks solid. If that comment doesn't generate any changes, I would say this is merge ready. |
|
I'd like to see this implemented too! |
| @@ -81,12 +83,32 @@ class HistoryAccessor(Configurable): | |||
| ipython --HistoryManager.hist_file=/tmp/ipython_hist.sqlite | |||
|
|
|||
| """) | |||
|
|
|||
| disabled = Bool(False, config=True, | |||
There was a problem hiding this comment.
Just one issue that I see: I tend to prefer config variables that are positive, to avoid having to write code with double-negatives. if not disabled is harder to parse mentally than if enabled, and for the other case, if not enabled is just about the same mental load as if disabled. So I would vote for changing this one to have the config variable be enabled.
I know it will require flipping all the logic in the PR around, but it's easy work. Happy to discuss if you disagree, but let's go over it before merging...
There was a problem hiding this comment.
I guess I am +1 on Fernando's comment about disabled->enabled.
On Fri, Aug 10, 2012 at 12:59 PM, Fernando Perez
notifications@github.comwrote:
In IPython/core/history.py:
@@ -81,12 +83,32 @@ class HistoryAccessor(Configurable):
ipython --HistoryManager.hist_file=/tmp/ipython_hist.sqlite""")
- disabled = Bool(False, config=True,
Just one issue that I see: I tend to prefer config variables that are
positive, to avoid having to write code with effectively double-negatives. if
not disabled is harder to parse mentally than if enabled, and for the
other case, if not enabled is just about the same mental load as if
disabled. So I would vote for changing this one to have the config
variable be enabled.I know it will require flipping all the logic in the PR around, but it's
easy work. Happy to discuss if you disagree, but let's go over it before
merging...—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2278/files#r1354995.
Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com
There was a problem hiding this comment.
I was 50-50 while writing, so I will make that change shortly.
The reason I went with disabled is that ~all people for whom this is relevant are looking for a way to 'disable' the history, so disable is the relevant active verb for users, even of 'enable' is cleaner for the object itself.
-MinRK
On Aug 10, 2012, at 13:25, "Brian E. Granger" notifications@github.com wrote:
In IPython/core/history.py:
@@ -81,12 +83,32 @@ class HistoryAccessor(Configurable):
ipython --HistoryManager.hist_file=/tmp/ipython_hist.sqlite""")
- disabled = Bool(False, config=True,
I guess I am +1 on Fernando's comment about disabled->enabled. On Fri, Aug 10, 2012 at 12:59 PM, Fernando Perez notifications@github.comwrote:
…
—
Reply to this email directly or view it on GitHub.
|
Test results for commit 5ed8fae merged into master (2092555)
Extra args: ['-x'] |
|
|
|
Great, thanks! Merging now, excellent job. |
allow disabling SQLite history * adds HistoryAccessor.disabled configurable for turning off the SQLite history * also provides connection_options configurable dict for relaying kwargs to sqlite3.connect. This is a just-in-case measure, but I think valuable if people have a reason to adjust the SQLite connection flags. Mainly for use in situations where threading or filesystem issues can cause problems for sqlite, e.g. embedding in django. closes #2276
allow disabling SQLite history * adds HistoryAccessor.disabled configurable for turning off the SQLite history * also provides connection_options configurable dict for relaying kwargs to sqlite3.connect. This is a just-in-case measure, but I think valuable if people have a reason to adjust the SQLite connection flags. Mainly for use in situations where threading or filesystem issues can cause problems for sqlite, e.g. embedding in django. closes ipython#2276
Mainly for use in situations where threading or filesystem issues can cause problems for sqlite, e.g. embedding in django.
closes #2276