-
-
Notifications
You must be signed in to change notification settings - Fork 76
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: Launch first console instance only if server is up #65
Conversation
spyder_terminal/terminalplugin.py
Outdated
QTimer.singleShot(250, self.__wait_server_to_start) | ||
elif code == 200: | ||
self.create_new_term(give_focus=False) | ||
self.sig_server_is_up.emit() |
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.
This signal seems unnecessary, I mean, nothing is connected to it.
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 will remove it, then!
spyder_terminal/terminalplugin.py
Outdated
try: | ||
code = requests.get('http://127.0.0.1:{0}'.format( | ||
self.port)).status_code | ||
except Exception: |
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.
Remove Exception
here because it's not needed.
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.
It is needed, when the server is unavailable it actually launches an Exception
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.
requests.exceptions.ConnectionError: HTTPConnectionPool(host='127.0.0.1', port=8000):
Max retries exceeded with url: /
(Caused by NewConnectionError('<requests.packages.urllib3.connection.HTTPConnection object at 0x7fd0b06a3390>:
Failed to establish a new connection: [Errno 111] Connection refused',))
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.
Yeah, but if you only use
try:
...
except:
...
then all exceptions will be captured.
Looks good now, thanks @andfoy! |
@@ -82,8 +85,9 @@ def test_new_terminal(qtbot): | |||
"""Test if a new terminal is added.""" | |||
# Setup widget | |||
terminal = setup_terminal(qtbot) | |||
term = terminal.get_current_term() | |||
# terminal.close_term() |
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.
Remove comment
@@ -70,6 +72,7 @@ def test_terminal_font(qtbot): | |||
def test_terminal_tab_title(qtbot): | |||
"""Test if terminal tab titles are numbered sequentially.""" | |||
terminal = setup_terminal(qtbot) | |||
# terminal.close_term() |
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.
Remove comment
@@ -56,8 +56,10 @@ def setup_terminal(qtbot): | |||
def test_terminal_font(qtbot): | |||
"""Test if terminal loads a custom font.""" | |||
terminal = setup_terminal(qtbot) | |||
term = terminal.get_current_term() | |||
# terminal.close_term() |
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.
Remove comment
spyder_terminal/terminalplugin.py
Outdated
' please restart Spyder on debugging mode ' | ||
"and open an issue with the contents of " | ||
"<tt>{1}</tt> and <tt>{2}</tt> " | ||
"files.".format(self.port, |
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.
Let's improve this a bit
files at {3}.
where {3}
is
https://github.com/spyder-ide/spyder-terminal/issues
I left a couple of comments, then I think this one is ready. |
Fixes #59