Move data summary setting from "positron" to "dataExplorer" #7263

kv9898 · 2025-04-15T14:49:11Z

Release Notes

New Features

N/A

Bug Fixes

Ideally, the two setting entries should be under "explorer" rather than "positron" so the user can find them more easily

QA Notes

N/A

Signed-off-by: Dianyi Yang <[email protected]>

wesm · 2025-04-17T19:22:39Z

Might make sense to have a positron.dataexplorer.* settings namespace or similar. I think we probably still want a positron. settings prefix

kv9898 · 2025-04-17T23:15:57Z

Might make sense to have a positron.dataexplorer.* settings namespace or similar. I think we probably still want a positron. settings prefix

It might make sense to use a positron.dataexplorer.* settings namespace or something similar. I do agree that keeping a positron. prefix helps distinguish Positron-specific configuration settings from VS Code-wide ones.

That said, this PR intentionally uses the explorer. prefix so that the data summary settings appear alongside existing Explorer settings in the Settings UI. This allows users to easily find them under Features > Explorer using the left-hand navigation tree.

If we were to use the positron. prefix here, these settings would be isolated and potentially hard to discover at this stage. I’d suggest deferring the broader shift to a unified positron. prefix for all Positron-specific settings to a future PR.

Here is how it looks with the current PR:

juliasilge · 2025-04-18T16:13:25Z

I'll quote from our internal docs here (not available currently for external contributors -- apologies!), for some guidance:

Do not include terms like “Positron”, “Setting”, “Extension”, or “Configuration”, in the title or display name. These terms do not provide information to the user that they don’t already know, e.g. “Kernel Supervisor” is preferred over “Positron Kernel Supervisor Settings”.

kv9898 · 2025-04-18T16:51:00Z

@juliasilge The doc seems to be in this PR's favour over my original naming!

Just to avoid any confusion, the original naming (positron.dataExplorerSummary*) also came from me through #6519, when I did not know about Positron's configuration naming conventions. Later, I found that the configurable settings I added were not placed where they should be (i.e. with other Explorer ones). Hence this PR.

Glad to know that this PR accidentally follows Positron's internal naming convention, as you just mentioned. This saves some hassle as in another PR of mine (#7266), which is awaiting review, also uses the explorer.* format.

softwarenerd

Commenting inline.

kv9898

Commenting inline.

👋 I saw you requested changes and mentioned inline comments — just checking in case they didn’t get submitted properly (I didn't see any comments). Happy to revise once I can see the feedback!

softwarenerd

I've made some comments in the PR. These changes shouldn't be made as they are. We will review the use of Positron: Some Setting Name internally and decide what we're going to do about this.

src/vs/workbench/services/positronDataExplorer/browser/positronDataExplorerSummary.ts

softwarenerd · 2025-04-22T17:18:38Z

src/vs/workbench/services/positronDataExplorer/browser/positronDataExplorerSummary.ts

@@ -16,9 +16,9 @@ import { PositronDataExplorerLayout } from './interfaces/positronDataExplorerSer

 // Key for the configuration setting
 export const USE_POSITRON_DATA_EXPLORER_SUMMARY_COLLAPSED_KEY =
-	'positron.dataExplorerSummaryCollapsed';
+	'explorer.summaryCollapsed';


This is not an explorer setting. It's a dataExplorer setting, so the name should be dataExplorerSummaryCollapsed. Whether or not it should be prefixed with positron should be reviewed, but at the moment, we have many settings that appear as Positron: Remote Host Experimental, and so on. So this change is not approved as it is.

Ahh this makes sense. But, the reason I made this change was that Explorer was the closest entry I could found in the tree view (see the screenshot in #7263 (comment)) If we do dataExplorer, the user won't be able to find it from the tree, unless we add a separate dataExplorer to the tree view.

I'm looking into this. FYI, Explorer settings are for the left pane EXPLORER panel:

Such as:

softwarenerd · 2025-04-22T17:19:05Z

src/vs/workbench/services/positronDataExplorer/browser/positronDataExplorerSummary.ts

 export const USE_POSITRON_DATA_EXPLORER_SUMMARY_LAYOUT_KEY =
-	'positron.dataExplorerSummaryLayout';
+	'explorer.summaryLayout';


This is not an explorer setting. It's a dataExplorer setting, so the name should be dataExplorerSummaryLayout. Whether or not it should be prefixed with positron should be reviewed, but at the moment, we have many settings that appear as Positron: Remote Host Experimental, and so on. So this change is not approved as it is.

softwarenerd · 2025-04-22T17:27:24Z

Commenting inline.

👋 I saw you requested changes and mentioned inline comments — just checking in case they didn’t get submitted properly (I didn't see any comments). Happy to revise once I can see the feedback!

Hi. I approved the PR by mistake, so I had to undo this. Please see my comments for the individual changed lines.

For Positron-specific localizations, we prefix every key with positron., so for example, we might have positron.someStringName. We do this so that we can find our localizations.

As for the settings, they are dataExplorer settings, not explorer settings, so they should be named dataExplorerSummaryCollapsed and dataExplorerSummaryLayout. Whether Positron settings should be prefixed with positron needs to be reviewed internally. We have many of these right now.

Thanks!

kv9898 · 2025-04-22T17:41:34Z

@softwarenerd Sure! I just made the changes (I allowed edits by maintainers so feel free to overwrite my code). My worry was that if we switched to the dataExplorer prefix, the users would not be able to easily find it (i.e. from the tree). I think, in that case, it would be worth adding the dataExplorer to the tree, since there is another PR of mine (#7266) which belongs to this group.

softwarenerd · 2025-04-22T19:10:11Z

@softwarenerd Sure! I just made the changes (I allowed edits by maintainers so feel free to overwrite my code). My worry was that if we switched to the dataExplorer prefix, the users would not be able to easily find it (i.e. from the tree). I think, in that case, it would be worth adding the dataExplorer to the tree, since there is another PR of mine (#7266) which belongs to this group.

Hi @kv9898, I've made a few adjustments to the settings and added a new Data Explorer feature area to the table of contents for settings. I've pushed these changes to your branch / PR.

The result:

Your PR helped us clarify these settings. Thanks!

* Fix typos * Mimic R's scipen in data explorer * update setting names in line with posit-dev#7263

softwarenerd

LG!

kv9898 · 2025-04-24T01:42:07Z

@softwarenerd please can you merge this in, since I do not have write access?

* Fix typos * Mimic R's scipen in data explorer * update setting names in line with posit-dev#7263

juliasilge · 2025-04-25T15:35:18Z

I opened #7390 to apply these commits on a non-fork PR, because of the problems outlined in #6628.

Thank you so much, @kv9898!

kv9898 · 2025-04-25T15:39:55Z

@juliasilge Many thanks! Please may I have my other PR (#7390) assigned a reviewer when convenient?

@kv9898

This is an internal, non-fork PR applying the commits from #7263, because of the problems outlined in #6628 Looking good! ![Screenshot 2025-04-25 at 9 32 32 AM](https://github.com/user-attachments/assets/e49459da-c234-43d1-b840-6eaba45aa982) ### Release Notes #### New Features - N/A #### Bug Fixes - Improved discoverability of Data Explorer settings, thanks to @kv9898 --------- Signed-off-by: Dianyi Yang <[email protected]> Co-authored-by: Jonathan McPherson <[email protected]> Co-authored-by: Dianyi Yang <[email protected]> Co-authored-by: Dianyi Yang <[email protected]> Co-authored-by: Brian Lambert <[email protected]>

juliasilge · 2025-04-25T17:20:12Z

Closed by #7390

Fix data summary setting names

1011689

Signed-off-by: Dianyi Yang <[email protected]>

petetronic requested a review from softwarenerd April 15, 2025 15:41

softwarenerd approved these changes Apr 22, 2025

View reviewed changes

softwarenerd requested changes Apr 22, 2025

View reviewed changes

kv9898 commented Apr 22, 2025

View reviewed changes

softwarenerd requested changes Apr 22, 2025

View reviewed changes

Revert localized entries to original

b25f4a8

Switch to dataExplorer

793e700

kv9898 requested a review from softwarenerd April 22, 2025 17:44

kv9898 changed the title ~~Move data summary setting from "positron" to "explorer"~~ Move data summary setting from "positron" to "dataExplorer" Apr 22, 2025

Updates to Data Explorer settings

d9f609a

Move Data Explorer to be after Explorer in TOC

7c588a4

kv9898 added a commit to kv9898/positron that referenced this pull request Apr 22, 2025

update setting names in line with posit-dev#7263

ba39d16

Merge branch 'posit-dev:main' into patch-1

27a3d47

kv9898 added a commit to kv9898/positron that referenced this pull request Apr 23, 2025

Scipen (#4)

5740c79

* Fix typos * Mimic R's scipen in data explorer * update setting names in line with posit-dev#7263

softwarenerd approved these changes Apr 23, 2025

View reviewed changes

kv9898 added a commit to kv9898/positron that referenced this pull request Apr 24, 2025

Scipen (#4)

56a2627

* Fix typos * Mimic R's scipen in data explorer * update setting names in line with posit-dev#7263

juliasilge mentioned this pull request Apr 25, 2025

Move data summary setting from "positron" to "dataExplorer" #7390

Merged

juliasilge closed this Apr 25, 2025

github-actions bot locked and limited conversation to collaborators Apr 25, 2025

kv9898 deleted the patch-1 branch April 27, 2025 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move data summary setting from "positron" to "dataExplorer" #7263

Move data summary setting from "positron" to "dataExplorer" #7263

kv9898 commented Apr 15, 2025

wesm commented Apr 17, 2025

kv9898 commented Apr 17, 2025

juliasilge commented Apr 18, 2025

kv9898 commented Apr 18, 2025 •

edited

Loading

softwarenerd left a comment

kv9898 left a comment

softwarenerd left a comment

softwarenerd Apr 22, 2025

kv9898 Apr 22, 2025 •

edited

Loading

softwarenerd Apr 22, 2025 •

edited

Loading

softwarenerd Apr 22, 2025

softwarenerd commented Apr 22, 2025

kv9898 commented Apr 22, 2025

softwarenerd commented Apr 22, 2025 •

edited

Loading

softwarenerd left a comment

kv9898 commented Apr 24, 2025

juliasilge commented Apr 25, 2025

kv9898 commented Apr 25, 2025

juliasilge commented Apr 25, 2025

Move data summary setting from "positron" to "dataExplorer" #7263

Move data summary setting from "positron" to "dataExplorer" #7263

Conversation

kv9898 commented Apr 15, 2025

Release Notes

New Features

Bug Fixes

QA Notes

wesm commented Apr 17, 2025

kv9898 commented Apr 17, 2025

juliasilge commented Apr 18, 2025

kv9898 commented Apr 18, 2025 • edited Loading

softwarenerd left a comment

Choose a reason for hiding this comment

kv9898 left a comment

Choose a reason for hiding this comment

softwarenerd left a comment

Choose a reason for hiding this comment

softwarenerd Apr 22, 2025

Choose a reason for hiding this comment

kv9898 Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

softwarenerd Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

softwarenerd Apr 22, 2025

Choose a reason for hiding this comment

softwarenerd commented Apr 22, 2025

kv9898 commented Apr 22, 2025

softwarenerd commented Apr 22, 2025 • edited Loading

softwarenerd left a comment

Choose a reason for hiding this comment

kv9898 commented Apr 24, 2025

juliasilge commented Apr 25, 2025

kv9898 commented Apr 25, 2025

juliasilge commented Apr 25, 2025

kv9898 commented Apr 18, 2025 •

edited

Loading

kv9898 Apr 22, 2025 •

edited

Loading

softwarenerd Apr 22, 2025 •

edited

Loading

softwarenerd commented Apr 22, 2025 •

edited

Loading