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: Refactor the way clients are created (IPython console) #19062

Merged
merged 35 commits into from
Sep 19, 2022

Conversation

impact27
Copy link
Contributor

@impact27 impact27 commented Aug 14, 2022

Description of Changes

  • fixes completion while debugging for remote kernels
  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

Fixes #

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:

@pep8speaks
Copy link

pep8speaks commented Aug 14, 2022

Hello @impact27! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 1645:1: W293 blank line contains whitespace

Line 1348:17: E124 closing bracket does not match visual indentation

Comment last updated at 2022-09-13 08:19:10 UTC

@impact27 impact27 force-pushed the refactor_createClientWidget branch 8 times, most recently from c3f5a29 to 9f63ef6 Compare August 16, 2022 07:50
@impact27
Copy link
Contributor Author

impact27 commented Aug 16, 2022

  • remove sig_external_spyder_kernel_connected and related methods
    This is the same as sig_spyder_kernel_checked so no need to have two signals for the same use
  • remove set_client_elapsed_time
    This function is used only in create_client_for_kernel and needlessly loops on all clients when the related clients are already found in create_client_for_kernel
  • remove kernel_id
    Unused as far as I can tell
  • rename configure_shellwidget to connect_shellwidget_signals
    Better description of the method
    Also remove give_focus argument because it is already set in the constructor
  • remove set_exit_callback
    No point in having it be a method
  • rename send_kernel_configuration to send_spyder_kernel_configuration
    This configuration uses comms and therefore is spyder-kernel specific (not for ipykernel)
  • remove get_kernel
    Almost unused and misleading name
  • Change is_external_kernel
    Now simply checks if kernel_manager is None, so it is consistant
  • Move 'Restart kernel?' QMessageBox to main_widget
    simplifies the code and conceptually better
  • Avoid double call to set_color_scheme when restarting kernel
  • Simplify ClientWidget constructor
    • Many arguments are simply read from config, and can be read from the constructor:
      • history_filename
      • show_elapsed_time
      • reset_warning
      • css_path
    • Arguments that are really main_widget settings and should not be in ClientWidget:
      • ask_before_restart
      • ask_before_closing
    • Arguments that are kernel-related and bundled with the kernel:
      • is_spyder_kernel
      • is_external_kernel
      • connection_file
      • stderr_obj
      • stdout_obj
      • fault_obj
  • The kernel connection is simplified:
    • create a kernel dictionnary to keep all kernel-related objects
    • create a connect_kernel on ClientWidget and ShellWidget to replace connect_client_to_kernel and set_kernel_client_and_manager

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @impact27 for your patience with this one! Other than my lightweight comments and suggestions, I think this should be ready.

spyder/plugins/ipythonconsole/utils/kernel_handler.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/utils/ssh.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/utils/kernel_handler.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/client.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/shell.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/shell.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/shell.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/utils/kernel_handler.py Outdated Show resolved Hide resolved
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @impact27!

@ccordoba12 ccordoba12 merged commit 2aaf1e0 into spyder-ide:master Sep 19, 2022
jitseniesen added a commit to jitseniesen/spyder-notebook that referenced this pull request May 8, 2023
Before, we passed the kernel ID when calling `create_client_for_kernel()` in the IPython
Console plugin. This worked, even though that function expected the connection file, because
that function used `find_connection_file()` to fix the path to the connection file. After the
refactor in PR spyder-ide/spyder#19062, the argument passed for the connection file is
expected to be really the file path and no attempt to fix it is made, so now we have to
really pass the connection file.
jitseniesen added a commit to jitseniesen/spyder-notebook that referenced this pull request May 25, 2023
Before, we passed the kernel ID when calling `create_client_for_kernel()` in the IPython
Console plugin. This worked, even though that function expected the connection file, because
that function used `find_connection_file()` to fix the path to the connection file. After the
refactor in PR spyder-ide/spyder#19062, the argument passed for the connection file is
expected to be really the file path and no attempt to fix it is made, so now we have to
really pass the connection file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants