-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added context menu option to reset IPython namespace #2997
Conversation
@webcsm, thanks a lot for your contribution. I have a couple of comments after a quick review:
|
Thank you for your patience in showing me the problems with my PR. I was not sure about which files I should change and well... about the removed trailing spaces, it ended up being a configuration of my Spyder that I had to turn off. Another thing is that I put "%reset -f" in the function, which forces the reset without asking for confirmation. That's what I would expect from this option. What do you think about that? |
I mean, resetting your namespace is quite a big deal, especially if you've loaded a lot of data. So if you select this new option by mistake, you could be very angry :-) |
I agree with @ccordoba12, destructive actions should always have a question :-p, something like reply = QMessageBox.question(
None,
"Clear namespace for console XX",
"Are you sure you want to clean the namespace?",
QMessageBox.Yes | QMessageBox.No,
)
if reply == QMessageBox.No:
return |
None, | ||
_("Reset IPython namespace"), | ||
_("All user-defined variables will be removed." | ||
"<br>Are you sure you want to clean the namespace?"), |
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 change clean
here to reset
(to be in line with the %reset
magic).
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 mean, this line should read
Are you sure you want to reset the namespace?
Thank you for your suggestions @goanpeca and @ccordoba12. I've fixed the problems in the last commit. |
👍 thanks for the work on this :-) @webcsm |
Thanks lot for keep working on this @webcsm. Everything is shaping nicely! There's only one little thing before merging: the new shortcut ( For that you need to check how we add other shortcuts, in the |
Hey guys, I had to create the shortcut and then call get_shortcut in add_actions_to_context_menu. Please, check if I did everything right. |
Now things are failing with this error: Traceback (most recent call last):
File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/ipythonconsole.py", line 855, in create_new_client
menu_actions=self.menu_actions)
File "/home/carlos/Projects/spyder/github-repo/spyderlib/widgets/ipython.py", line 415, in __init__
local_kernel=False)
File "/home/carlos/Projects/spyder/github-repo/spyderlib/widgets/ipython.py", line 188, in __init__
self.shortcuts = self.create_shortcuts()
File "/home/carlos/Projects/spyder/github-repo/spyderlib/widgets/ipython.py", line 256, in create_shortcuts
reset_namespace = create_shortcut(self.reset_namespace, context='Console',
AttributeError: 'IPythonShellWidget' object has no attribute 'reset_namespace' This mean that you need to move the current For the contex menu action to work, you also need to define a new def reset_namespace(self):
self.shellwidget.reset_namespace() I know this is a little bit confuse, but the idea is that you need two If you read the code you will notice that this is what we do for |
Don't worry @ccordoba12. I worked on an open-source project using Qt for 7 years and I know how things are. I'm commiting now the fix. I took the liberty to make the clear console method within the client call the method in the shell widget too so that the actual code is encapsulated in just one function. |
One of the tests failed. I don't know why. Is that normal? |
There are still some failures:
I noticed those problem while live testing your branch :-) In short, this is the diff of the last change you need to do to have everything working: diff --git a/spyderlib/widgets/ipython.py b/spyderlib/widgets/ipython.py
index 59b5c42..9d6aee7 100644
--- a/spyderlib/widgets/ipython.py
+++ b/spyderlib/widgets/ipython.py
@@ -267,17 +267,16 @@ These commands were executed:
parent=self)
clear_console = create_shortcut(self.clear_console, context='Console',
name='Clear shell', parent=self)
- reset_namespace = create_shortcut(self.reset_namespace, context='Console',
- name='Reset namespace', parent=self)
# Fixed shortcuts
new_shortcut("Ctrl+T", self, lambda: self.new_client.emit())
+ new_shortcut("Ctrl+R", self, lambda: self.reset_namespace())
new_shortcut(SHORTCUT_INLINE, self,
lambda: self._control.enter_array_inline())
new_shortcut(SHORTCUT_TABLE, self,
lambda: self._control.enter_array_table())
- return [inspect, clear_console, reset_namespace]
+ return [inspect, clear_console]
def clean_invalid_var_chars(self, var):
"""
@@ -601,8 +600,7 @@ class IPythonClient(QWidget, SaveHistoryMixin):
icon=ima.icon('editdelete'),
triggered=self.clear_line)
reset_namespace_action = create_action(self, _("Reset namespace"),
- QKeySequence(get_shortcut('console',
- 'reset namespace')),
+ QKeySequence("Ctrl+R"),
triggered=self.reset_namespace)
clear_console_action = create_action(self, _("Clear console"),
QKeySequence(get_shortcut('console', |
Sorry if it took more time than it should. It's rather difficult to change a code that you don't know very well. |
Don't worry about it :-) It wasn't my intention to be harsh, and I hope my comments don't discourage you to keep contributing to Spyder ;-) I just noticed that you didn't test your changes after doing them (else you'd have noticed that new errors keep appearing :-) And to do live testing, most of the core developers start Spyder (from the root of our git clones) with this command:
That avoids installing Spyder in a virtualenv or conda env, and keep re-installing it after every change. So my advice for next time is: please do live testing and verify that things are working as expected. Our continuous integration testing is not fine-grained enough to detect that things like shortcuts and context-menu actions are working correctly. And even though @goanpeca and me tried to guide you the best we could, you noticed we also made our blunders, and were not right all the time. So live testing is the cure for that :-) |
And finally, thanks for your contribution! |
Added context menu option to reset IPython namespace
Hey @ccordoba12. Thanks for your tip. I thought that it would give me some trouble to set up the environment for testing... and I'm doing this in my spare time. But following your tip I quickly got everything set up. |
👍 |
Fixes #2986
Fixes #3000
Hi @ccordoba12! This is my first attempt to help you guys. Tell me if I need to fix anything else in the PR. If this goes through, maybe I'll tackle the vertical lines thing. #2986