-
Notifications
You must be signed in to change notification settings - Fork 65
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
JohanMabille
merged 4 commits into
jupyter:master
from
brichet:debugger_info_copy_to_globals
Dec 11, 2023
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
2cc1736
JEP for copy-to-globals support in debugger_info
brichet cadd465
Add requests description
brichet 16ef933
Update debugger-info-copy-to-globals/debugger-info-copy-to-globals.md
brichet 109c920
Update debugger-info-copy-to-globals/debugger-info-copy-to-globals.md
brichet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Add requests description
- Loading branch information
commit cadd4651dda88ba78075d28f840b16131af10ad5
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Would not it be more in line with DAP to advertise it via
capabilities
ofinitialize
request response? For example, "copy to clipboard" can be implemented relying onsupportsClipboardContext?: boolean;
.I see that this would require modifying the message in flight, but it seems cleaner on the API side.
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 guess the same goes for
richRendering
(which we have not codified so far).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 guess the same goes for
richRendering
(which we have not codified so far).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 @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 thedebugInfo
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.
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'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.