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

JEP for copy-to-globals support in debugger_info #93

Merged
merged 4 commits into from
Dec 11, 2023
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions debugger-info-copy-to-globals/debugger-info-copy-to-globals.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
---
title: Debugger support to copyToGlobals
authors: Nicolas Brichet (@brichet)
issue-number: xxx
pr-number: xxx
date-started: 2023-02-20
---

# Summary

This JEP introduces a new field to the kernel debugger_info response. This new
field will inform the UI that the debugger supports the `copyToGlobals` request.

# Motivation

The `copyToGlobals` request has been introduced in
[ipykernel](https://github.com/ipython/ipykernel/pull/1055) and in
[xeus-python](https://github.com/jupyter-xeus/xeus-python/pull/562) to copy a local
variable to the global scope during a breakpoint. It would be useful to inform the
UI if this is supported by the kernel before displaying the corresponding menu entry.

# Proposed Enhancement

We propose to add a new `copyToGlobals` boolean field to the `debugger_info`
Copy link
Member

Choose a reason for hiding this comment

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

Would not it be more in line with DAP to advertise it via capabilities of initialize request response? For example, "copy to clipboard" can be implemented relying on supportsClipboardContext?: boolean;.

I see that this would require modifying the message in flight, but it seems cleaner on the API side.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the same goes for richRendering (which we have not codified so far).

Copy link
Member

Choose a reason for hiding this comment

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

I guess the same goes for richRendering (which we have not codified so far).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @krassowski

These "capabilities" are really kernel capabilities, and not debugger capabilities. I'm more in favor of separating the 2 types of capabilities, for clarity. An other solution could be to add a capabilities object in the debugInfo response, but that would not be backward compatible.

If I am not mistaken, all requests/responses defined in DAP are only forwarded by the kernel. It is just an interface between UI and debugger. Changes to the DAP message format will only affect the UI and not the kernels, making maintenance easier.

Copy link
Member

@JohanMabille JohanMabille Feb 23, 2023

Choose a reason for hiding this comment

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

Would not it be more in line with DAP to advertise it via capabilities of initialize request response? For example, "copy to clipboard" can be implemented relying on supportsClipboardContext?: boolean;.

I'm not sure. The idea until now was to extend the existing DAP by adding new messages, not by modifying the existing messages or types used to compose the messages. Adding new capabilities to the existing ones in the DAP would mean introducing a new interface (inheriting form the Capabilities one) and replacing the InitializeResponse spec with our own. From a protocol side, it is easier to maintain a clear separation between the DAP original specification, and our adds to it.

response which will inform the UI that this request is supported.

## Reference-level explanation

This boolean flag should be included in the `debugger_info` response from the kernel
which supports the feature. It is optional, assuming that its absence is understood
as `false` from the client perspective.

If the feature is supported, the kernel must provide a function for copying a variable
from a local scope to the global scope.
The communication between the UI and the kernel (request - response) will have the
structures described at
https://jupyter-client.readthedocs.io/en/latest/messaging.html#copytoglobals.

- Request (from UI to kernel)

```json
brichet marked this conversation as resolved.
Show resolved Hide resolved
{
'type': 'request',
'command': 'copyToGlobals',
'arguments': {
# the variable to copy from the frame corresponding to `srcFrameId`
'srcVariableName': str,
'srcFrameId': int,
# the copied variable name in the global scope
'dstVariableName': str
}
}
```

- Response (from kernel to UI)

```json
brichet marked this conversation as resolved.
Show resolved Hide resolved
{
'type': 'response',
'success': bool,
'command': 'setExpression',
'body': {
# string representation of the copied variable
'value': str,
# type of the copied variable
'type': str,
'variablesReference': int
}
}
Loading