-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Inoculate clearcmd extension into %reset functionality #1309
Conversation
Previously, the %clear completions did not advertise the 'array' argument option, which is now fixed.
A few thoughts:
|
self.history_manager.input_hist_raw[:] = ['\n'] * pc | ||
user_ns.update(dict(_i=u'',_ii=u'',_iii=u'')) | ||
del self.history_manager.input_hist_parsed[:] | ||
del self.history_manager.input_hist_raw[:] |
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.
I suspect this probably disrupts some things that assume the input history from the session is consistent. I don't see any reason to clear input history, anyway.
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.
Well, the way i see it, there are no failing tests - so it's acceptable. If this will cause something to break, we should have a test that ensures that whatever this will cause to break isn't broken. See my 'elvis' example for a use case of this.
Thanks for taking a look, Thomas.
does this mean that shadow_nuke and shadow_compress don't really do anything anymore and should be eliminated?
I was actually wondering why
Well, depends on the use case. I can imagine someone pasting in 100s of MB of protein sequences as string literals and then just doing
I was just trying to revive this code to eliminate a "KnownFailure" in |
On 23 January 2012 23:34, Paul Ivanov reply@reply.github.com wrote:
Yes, I think so.
If I do: In [1]: print "foo" foo In [2]: %clear in In [3]: print "bar" Then I think we can 'clear' it without messing up the history numbers by doing
|
It seems GH doesn't like my markdown today. Hopefully it's clear enough. |
@takluyver - crystal - makes sense, adding your test now and reverting the ['']*len functionality |
ok, I've addressed the points Thomas brought up, here's what's left to figure out:
|
I honestly don't remember what their purpose was when they were introduced. I suggest doing a careful grin code-wide to see if anything turns up that uses them. If it's only in quarantine or deathrow, we'll have a look. If there's nothing, nuke them. And if any significant use case turns up, we'll also have a more careful look.
I'm fine with it being 'native', I think it was an extension also just for accidental historical reasons. |
this.
Other than that, shadowhist isn't referenced or used anywhere else in the entire code. I'll just nuke it an remove the comments from the excerpt above.
oops, turns out there's already a
I like 3. |
Shadow history was a secondary history system that stored all your input, but in no particular order (like a persistent set), when the main history system only stored a limited number of entries. I combined the two when I rewrote history to use SQLite. At some point, we'll need some functionality to truncate the SQLite history, but it's low priority, and the code will be nothing like that for shadow history. I prefer 2 (integrating it with reset). Both are about releasing data stored in memory. Running all of the 'clear' options here gets you close to what %reset does. It seems much more confusing to combine the purely aesthetic 'clear screen' with this 'clear data'. |
On Tue, Jan 24, 2012 at 6:01 AM, Thomas
Agreed: they should be grouped by functionality (namespace vs screen) |
this functionality used to live in extensions/clearcmd.py, and in PR ipython#1309, due to the fact that we already had a %clear magic for resetting the screen, it was decided that it'd be best to incorporate the functionality inside the %reset magic.
overruled :) I've moved incorporated this into the %reset magic. Also made the two reset magics reference one another via "See also". |
Great, thanks. I'll try to test this this evening. |
This seems to be working OK in Python 2.7 and 3.2. One remaining point - could we make the argument case insensitive? Because the variables are |
Thanks for taking a look, Thomas. Yeah, I thought about making How about I'll just make it case insensitive, but we keep the |
Sounds good to me. @fperez , this is good to go as far as I'm concerned. |
My only suggestion is to break up the |
ok, makes sense, done. |
Great, thanks. Running test suite now, if all passes will merge in a minute. Great job! |
Inoculate clearcmd extension into %reset functionality Restored the functionality of this quarantined module, added new tests for it and polished it a bit, and made it available as part of the `%reset` magic. Specifically, the `%clear` completions did not previously advertise the `array` argument option, and `clear in` did not clear `_i`, `_ii`, and `_iii` - both of these are now fixed. Removing the `shadow_compress` and `shadow_nuke` options, as they refer to functionality that does not appear to be used anywhere anymore.
I think this merge broke the sphinx doc generation:
I have no idea of sphinx so I can't really debug this. But if I remove all the % from the reset* lines in IPython/core/magic.py it works again. |
From the log, I note this is occurring in a function called
(GH will take the backticks as markdown - this should appear correctly by |
See PR #1338 |
this functionality used to live in extensions/clearcmd.py, and in PR ipython#1309, due to the fact that we already had a %clear magic for resetting the screen, it was decided that it'd be best to incorporate the functionality inside the %reset magic.
Inoculate clearcmd extension into %reset functionality Restored the functionality of this quarantined module, added new tests for it and polished it a bit, and made it available as part of the `%reset` magic. Specifically, the `%clear` completions did not previously advertise the `array` argument option, and `clear in` did not clear `_i`, `_ii`, and `_iii` - both of these are now fixed. Removing the `shadow_compress` and `shadow_nuke` options, as they refer to functionality that does not appear to be used anywhere anymore.
I have restored the functionality of this quarantined module, added new tests for it and polished it a bit, and made it available as part of the
%reset
magic.Specifically, the
%clear
completions did not previously advertise thearray
argument option, andclear in
did not clear_i
,_ii
, and_iii
- both of these are now fixed.I am removing the
shadow_compress
andshadow_nuke
options, as they refer to functionality that does not appear to be used anywhere anymore.update 2012-01-25
Ok, as was discussed bellow, I've moved incorporated this into the %reset magic.