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

Add setting showPythonInlineValues #407

Conversation

paulacamargo25
Copy link
Contributor

@paulacamargo25 paulacamargo25 commented Jul 24, 2024

Add setting for showing the inline values.
Closed: #410

@paulacamargo25 paulacamargo25 self-assigned this Jul 24, 2024
@paulacamargo25 paulacamargo25 added the feature-request Request for new features or functionality label Jul 24, 2024
@paulacamargo25 paulacamargo25 added this to the July 2024 milestone Jul 24, 2024
@paulacamargo25 paulacamargo25 force-pushed the add-setting-to-enable-inline-values branch from 1276df4 to be2a2ff Compare July 24, 2024 20:01
package.json Outdated Show resolved Hide resolved
Copy link
Member

@connor4312 connor4312 left a comment

Choose a reason for hiding this comment

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

Is there a request to disable/enable debug inline values on a per-language basis versus the base debug.inlineValues? If so this is something we should probably have in VS Code itself rather than an extension-specific setting

@paulacamargo25
Copy link
Contributor Author

Is there a request to disable/enable debug inline values on a per-language basis versus the base debug.inlineValues? If so this is something we should probably have in VS Code itself rather than an extension-specific setting

We are using this setting as experimental, there are some variables in the python inline values that we need to review and investigate more for the next iteration. So we decided to add this setting to have this value set to false by default.

@connor4312
Copy link
Member

It looks like debug.inlineValues functions in the current stable version of the Python extension, would a default value of false disable them for everyone?

@paulacamargo25
Copy link
Contributor Author

It looks like debug.inlineValues functions in the current stable version of the Python extension, would a default value of false disable them for everyone?

Yes, it will disable all the inline values

package.nls.json Outdated
@@ -4,5 +4,6 @@
"debugpy.command.debugUsingLaunchConfig.title": "Python Debugger: Debug using launch.json",
"debugpy.command.reportIssue.title": "Report Issue...",
"debugpy.command.viewOutput.title": "Show Output",
"debugpy.debugJustMyCode": "When debugging only step through user-written code. Disable this to allow stepping into library code."
"debugpy.debugJustMyCode": "When debugging only step through user-written code. Disable this to allow stepping into library code.",
"debugpy.showPythonInlineValues": "Enable this to see inline values in the editor while debugging."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"debugpy.showPythonInlineValues": "Enable this to see inline values in the editor while debugging."
"debugpy.showPythonInlineValues": "Whether to display inline values in the editor while debugging."

@connor4312
Copy link
Member

Yes, it will disable all the inline values

Is that intended? That seems like a regression for folks who were using them up until this point

@karthiknadig
Copy link
Member

@connor4312 This setting is similar to debug.inlineValues but it should not affect general one. The debugpy.showPythonInlineValues is added so we can explore the value filtering, via experimentation, till we get to a point where we can remove it. I agree that a more general purpose language specific version o debug.inlineValue is a core feature.

package.json Outdated
},
"debugpy.showPythonInlineValues": {
"default": false,
"description": "%debugpy.showPythonInlineValues%",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"description": "%debugpy.showPythonInlineValues%",
"description": "%debugpy.showPythonInlineValues.description%",

package.nls.json Outdated
@@ -4,5 +4,6 @@
"debugpy.command.debugUsingLaunchConfig.title": "Python Debugger: Debug using launch.json",
"debugpy.command.reportIssue.title": "Report Issue...",
"debugpy.command.viewOutput.title": "Show Output",
"debugpy.debugJustMyCode": "When debugging only step through user-written code. Disable this to allow stepping into library code."
"debugpy.debugJustMyCode": "When debugging only step through user-written code. Disable this to allow stepping into library code.",
"debugpy.showPythonInlineValues": "Whether to display inline values in the editor while debugging."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"debugpy.showPythonInlineValues": "Whether to display inline values in the editor while debugging."
"debugpy.showPythonInlineValues.description": "Whether to display inline values in the editor while debugging."

@connor4312
Copy link
Member

connor4312 commented Jul 25, 2024

@karthiknadig I have not tested the changes in this PR, but in VS Code if there is an inline value provider registered for a language then that will be used instead of the default handling that VS Code does, and is doing for Python stable. If I'm understanding it right, the implementation in this PR of just returning an empty array from the provider won't fall back to the old behavior, but will instead turn off inline values for everyone, including those who might have been using them previously.

https://github.com/microsoft/vscode/blob/06643dd85264bc88ab30acc007baad0db33b8c71/src/vs/workbench/contrib/debug/browser/debugEditorContribution.ts#L676

I think the better implementation is not registering an inline value provider at all if the setting is turned off so as not to regress functionality.

@karthiknadig
Copy link
Member

Thanks @connor4312 for the explanation. I agree that this should skip registration, if the setting is set to disable. If the setting was changed when the extension is already activated, we can just use the on settings change event and dispose the provider. registerInLineProvider API returns a disposable that we can use to register/unregister dynamically.

Copy link
Member

@connor4312 connor4312 left a comment

Choose a reason for hiding this comment

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

lgtm 👍


context.subscriptions.push(
workspace.onDidChangeConfiguration((event: ConfigurationChangeEvent) => {
if (event.affectsConfiguration('debugpy')) {
Copy link
Member

Choose a reason for hiding this comment

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

An aside but I have a ConfigValue helper that I've copied between multiple projects that makes this easier :)

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 for the suggestion Connor, I'll review it and add it as a dev week item.

@paulacamargo25 paulacamargo25 merged commit 36ec556 into microsoft:main Jul 25, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a setting to showPythonInlineValues
4 participants