-
Notifications
You must be signed in to change notification settings - Fork 139
WIP: Adds dataExplorer.useLanguagePackFormatOptions setting, to format the Data Explorer using R or Python options
#11326
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: kv9898 <105025148+kv9898@users.noreply.github.com>
Co-authored-by: kv9898 <105025148+kv9898@users.noreply.github.com>
Co-authored-by: kv9898 <105025148+kv9898@users.noreply.github.com>
Co-authored-by: kv9898 <105025148+kv9898@users.noreply.github.com>
Co-authored-by: kv9898 <105025148+kv9898@users.noreply.github.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
E2E Tests 🚀 |
Related to #11326 I think in posit-dev/ark#1001 I finally figured out what to put here, as we finally have the CLA passing in posit-dev/ark#987.
|
@dhruvisompura when you get a chance (no rush), can you do an initial glance at this PR and weigh in for me on what is a good way to address how the backend updating any of these options like
|
dhruvisompura
left a comment
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.
@dhruvisompura when you get a chance (no rush), can you do an initial glance at this PR and weigh in for me on what is a good way to address how the backend updating any of these options like
scipenshould play with the Data Explorer deciding to update, when updating an existing Data Explorer tab?
- We could try out triggering
updateBackendState()whenrequestFocus()is called to get fresh format options from the backend, but I worry about how expensive that is.- We could add a
getFormatOptions()backend method to call duringrequestFocus(), that only fetches format options, which would be cheaper than full state.- We could have the R/Python backends emit an event when the global format options change (e.g., when
options(scipen=...)is called) and the frontend could listen and invalidate the cached format options. I guess we could even update an existing Data Explorer tab with that?- Run away from home and live in the woods?
I am leaning towards option 3 where we have the R/Python backends emit an event when the global format options change so that open data explorer tabs can re-render with the new format options.
For option 1, calling updateBackendState() when requestFocus() is called does sound expensive and it doesn't handle the scenario where a format option is set while a data explorer tab is already open if I am understanding things correctly.
Option 2 doesn't address updating open Data Explorer tabs in the scenario where the user updates the setting after opening the Data Explorer, so we'd still need something like option 3.
I'd be curious to know what other folks thing about the backend emitting an event for this.
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.
Minor nit: I feel like this file could use a rename to make it clear that this contains the configuration options for the data explorer, especially given that USE_LANGUAGE_PACK_FORMAT_OPTIONS_KEY is not specific to the summary panel but the entirety of the data explorer.
Normally, we stick these settings in the contribution file or in the main service file which could be an option but calling it positronDataExplorerConfigurations.ts or something like that is probably good enough.
| default: false, | ||
| markdownDescription: localize( | ||
| 'positron.dataExplorerUseLanguagePackFormatOptions', | ||
| 'Use formatting options from the R or Python language pack when displaying data values. When enabled, settings like R\'s scipen option will affect number formatting in the Data Explorer.' |
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.
minor nit: we could be a bit more generic in this description and just say:
| 'Use formatting options from the R or Python language pack when displaying data values. When enabled, settings like R\'s scipen option will affect number formatting in the Data Explorer.' | |
| 'Use formatting options from the language pack when displaying data values. When enabled, settings like R\'s scipen option will affect number formatting in the Data Explorer.' |
I don't think we have to explicitly call out the language packs and this means one less thing to update if/when we support other languages.
This is a non-fork PR applying the commits from #11051 by @kv9898 because of the problems outlined in #6628. I agree with @dfalbel in the review on posit-dev/ark#987 that it is arguable whether R's defaults are better than what we have here for the Data Explorer (and we can't really tell if the user has changed from R defaults for these options), so I don't think we can use language pack options by default.
There is still a fair amount of work here to do, around:
@:data-explorer
Release Notes
New Features
dataExplorer.useLanguagePackFormatOptionssetting now supports formatting via R or Python optionsBug Fixes
QA Notes
WIP