Skip to content
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

PR: Handle errors while debugging code when importing tqdm #4019

Merged
merged 15 commits into from
Feb 25, 2017

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Jan 21, 2017

Fixes #4003


For now this only helps to prevent showing errors. However the debug function is not working properly with the tqdm import.

Now the debug function works but without showing colors.

This now only handles not showing errors when importing tqdm in debug mode. Also restarts the kernel to restore the debug functionality in the current kernel.

@ccordoba12
Copy link
Member

I like this, good job @dalthviz!

What's still missing from this PR?

@ccordoba12 ccordoba12 added this to the v3.1.2 milestone Jan 21, 2017
@dalthviz
Copy link
Member Author

@ccordoba12 the thing with this is that the changes in this PR only ensure that no error is thrown. However the debug output is not correct after you run the import line, this a preview of what you see:

imagen

Also, something strange is that if you press Enter in the console you can see the correct output but without colors:

imagen

@ccordoba12 ccordoba12 modified the milestones: v3.1.2, v3.1.3 Jan 22, 2017
@ccordoba12 ccordoba12 modified the milestones: v3.1.3, v3.1.4 Feb 8, 2017
@@ -12,10 +12,12 @@
import ast

from qtpy.QtCore import QEventLoop
from qtpy.QtWidgets import QMessageBox
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not needed anymore


from qtconsole.rich_jupyter_widget import RichJupyterWidget

from spyder.py3compat import to_text_string
from spyder.config.base import _
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is also not needed now :-)

@ccordoba12
Copy link
Member

This is looking really good. I think the only missing thing is a test for this ;-)

@dalthviz dalthviz changed the title [WIP]: Fix debugging code when importing tqdm PR: Fix debugging code when importing tqdm Feb 16, 2017
@dalthviz dalthviz changed the title PR: Fix debugging code when importing tqdm PR: Handle errors while debugging code when importing tqdm Feb 18, 2017
sw._append_html(_("<br>Kernel restarting because an "
"error raised while debugging "
"(stdout change)\n<hr><br>"),
before_prompt=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this message to

Restarting kernel because an error occurred while debugging

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this change again because it went away after I removed the test commits. Sorry.

if sw._input_none:
sw._append_html(_("<br>Kernel restarting because an "
"error raised while debugging "
"(stdout change)\n<hr><br>"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also change this message with the same one above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to redo this change.

@ccordoba12
Copy link
Member

ccordoba12 commented Feb 23, 2017

@dalthviz, I removed all commits related to tests in this PR. The thing is this error doesn't occur on Linux, only on Windows, and we can't run tests for the main window there :-/

try:
sw.kernel_manager.restart_kernel()
except RuntimeError as e:
if self.infowidget.isVisible():
Copy link
Member

@ccordoba12 ccordoba12 Feb 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block is indented 4 more lines than necessary. Please fix that.

@@ -25,7 +25,8 @@ class DebuggingWidget(RichJupyterWidget):
Spyder
"""

_input_reply = None
_input_reply = {}
_input_none = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this name to _input_reply_failed

@@ -40,6 +40,7 @@ class ShellWidget(NamepaceBrowserWidget, HelpWidget, DebuggingWidget):
sig_input_reply = Signal()
sig_pdb_step = Signal(str, int)
sig_prompt_ready = Signal()
sig_debug_restart = Signal()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this to sig_dbg_kernel_restart

@ccordoba12
Copy link
Member

@dalthviz, I left some other comments, mainly about naming conventions. Then this should be ready to be merged.

I really appreciate all the effort you have put to fix this problem. What a nasty bug!!

@@ -283,7 +284,7 @@ def _banner_default(self):

def _kernel_restarted_message(self, died=True):
msg = _("Kernel died, restarting") if died else _("Kernel restarting")
self.sig_kernel_restarted.emit(msg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change. This is not related to the things you're trying to solve in this PR.

@@ -176,6 +176,9 @@ def configure_shellwidget(self, give_focus=True):
# To show kernel restarted/died messages
self.shellwidget.sig_kernel_restarted.connect(
self.kernel_restarted_message)

Copy link
Member

@ccordoba12 ccordoba12 Feb 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment here explaining why this is needed.

@ccordoba12
Copy link
Member

@dalthviz, I finally managed to add tests for this, but in a different way than we had thought at the beginning :-)

@ccordoba12 ccordoba12 merged commit 6e17c60 into spyder-ide:3.1.x Feb 25, 2017
ccordoba12 added a commit that referenced this pull request Feb 25, 2017
ccordoba12 added a commit that referenced this pull request Feb 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants