-
-
Notifications
You must be signed in to change notification settings - Fork 40
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: Enable comms to work across different Python versions #492
Conversation
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.
Thanks @impact27 for your hard work on this! I only small formatting suggestions and a question for you.
@@ -346,14 +350,21 @@ def get_var_properties(self): | |||
return None | |||
|
|||
@comm_handler | |||
def get_value(self, name): | |||
def get_value(self, name, encoded=False): |
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.
Why encoded
is False
by default and not True
? Don't we need all values to be encoded with Cloudpickle?
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.
get_value is also used in spyder_kernels e.g. in the tests where there is no need for encoding. Only the comm call requires encoding. The extra bonus is that it makes the encoding explicit on the frontend side
Co-authored-by: Carlos Cordoba <[email protected]>
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.
Looks good to me now, thanks @impact27!
Cloudpickle only works when the exact same version of python is used. Replaced with json.
This creates a bit more work because all the input and output of comms must be json serialisable or bytes
Goes with spyder-ide/spyder#22120
Fixes #468
FrameSummary are not compatible between versions of python. This change follows the doc here: https://docs.python.org/3/library/traceback.html#traceback.FrameSummary