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.

Sign up for GitHub

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

Jump to bottom

PR: Improve validation for the right Spyder-kernels version #19074

Merged
merged 16 commits into from
Aug 25, 2022
Merged

PR: Improve validation for the right Spyder-kernels version #19074

merged 16 commits into from
Aug 25, 2022

Conversation

impact27
Copy link
Contributor

@impact27 impact27 commented Aug 16, 2022 •
edited by ccordoba12
Loading

Description of Changes

If the version of spyder and spyder kernels doesn't match, the user will see many errors. This PR show an error if spyder-kernels has an incompatible version.

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:

Quentin Peter added 3 commits August 16, 2022 08:51
git subrepo clone --branch=expose_spyder_kernels_version --force http…
1383988 
…s://github.com/impact27/spyder-kernels.git external-deps/spyder-kernels

subrepo:
  subdir:   "external-deps/spyder-kernels"
  merged:   "2dd5ad293"
upstream:
  origin:   "https://github.com/impact27/spyder-kernels.git"
  branch:   "expose_spyder_kernels_version"
  commit:   "2dd5ad293"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
spyder-kernels version
92ded35
add test check spyder kernels version
363f5c9
@impact27 impact27 mentioned this pull request Aug 16, 2022
Merged
3 tasks
@impact27 impact27 mentioned this pull request Aug 18, 2022
Merged
@ccordoba12 ccordoba12 added this to the v6.0alpha1 milestone Aug 18, 2022
ccordoba12
ccordoba12 reviewed Aug 18, 2022
View reviewed changes
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 work on this! I think it's a really good improvement because it'll allow us to easily check that users have the right Spyder-kernels version for remote kernels.

spyder/plugins/ipythonconsole/widgets/shell.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/shell.py Outdated Show resolved Hide resolved
spyder/utils/programs.py Outdated Show resolved Hide resolved
spyder/utils/programs.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/main_widget.py Show resolved Hide resolved
@ccordoba12 ccordoba12 changed the title PR: check spyder kernels version PR: Improve validation for the right Spyder-kernels version Aug 18, 2022
@impact27 impact27 mentioned this pull request Aug 18, 2022
Merged
Quentin Peter added 3 commits August 18, 2022 23:16
remove old warning
62b66cc
git subrepo clone --branch=add_executable_path --force https://github…
48c35da 
….com/impact27/spyder-kernels.git external-deps/spyder-kernels

subrepo:
  subdir:   "external-deps/spyder-kernels"
  merged:   "12d8b1183"
upstream:
  origin:   "https://github.com/impact27/spyder-kernels.git"
  branch:   "add_executable_path"
  commit:   "12d8b1183"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
fix test
b978703
@impact27 impact27 force-pushed the spyder_kernels_version branch from e5299f9 to b978703 Compare August 18, 2022 21:20
ccordoba12
ccordoba12 reviewed Aug 21, 2022
View reviewed changes
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 the update! It looks much better now.

spyder/plugins/ipythonconsole/widgets/shell.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/shell.py Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/shell.py 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
Quentin Peter and others added 3 commits August 21, 2022 22:41
git subrepo clone (merge) --branch=master --force https://github.com/…
897de82 
…spyder-ide/spyder-kernels.git external-deps/spyder-kernels

subrepo:
  subdir:   "external-deps/spyder-kernels"
  merged:   "8386ef242"
upstream:
  origin:   "https://github.com/spyder-ide/spyder-kernels.git"
  branch:   "master"
  commit:   "8386ef242"
git-subrepo:
  version:  "0.4.1"
  origin:   "https://github.com/ingydotnet/git-subrepo"
  commit:   "a04d8c2"
@impact27 @ccordoba12
Apply suggestions from code review
8196650 
Co-authored-by: Carlos Cordoba <[email protected]>
Update text
692b8e9
@ccordoba12
Copy link
Member

@impact27, there's a small regression with respect to the current functionality. If Spyder-kernels is not installed in an environment, users would see this message with your PR:

image

But this is what they see with the code in master:

image

Is there a way to preserve this behavior? Perhaps we could catch the ModuleNotFoundError and display instead the current message.

@pep8speaks
Copy link

pep8speaks commented Aug 22, 2022 •
edited
Loading

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

Line 69:80: E501 line too long (86 > 79 characters)

Comment last updated at 2022-08-25 02:28:08 UTC

@impact27 impact27 force-pushed the spyder_kernels_version branch 2 times, most recently from 402ed63 to 436bba9 Compare August 22, 2022 09:45
@impact27 impact27 force-pushed the spyder_kernels_version branch from 436bba9 to 5a5ae2b Compare August 22, 2022 09:46
ccordoba12
ccordoba12 reviewed Aug 24, 2022
View reviewed changes
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.

Last stylistic suggestions then this should be ready @impact27 (unless, of course, you plan to make further changes).

spyder/plugins/ipythonconsole/__init__.py Show resolved Hide resolved
spyder/plugins/ipythonconsole/utils/kernelspec.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/shell.py Outdated Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/shell.py Show resolved Hide resolved
ccordoba12
ccordoba12 approved these changes Aug 25, 2022
View reviewed changes
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.

Great work @impact27! This is a pretty nice improvement!

@ccordoba12 ccordoba12 merged commit 44182fc into spyder-ide:master Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccordoba12 ccordoba12 ccordoba12 approved these changes

Assignees

@impact27 impact27

Labels
None yet
Projects
None yet
Milestone
v6.0alpha1
Development

Successfully merging this pull request may close these issues.
3 participants
@impact27 @ccordoba12 @pep8speaks