-
Notifications
You must be signed in to change notification settings - Fork 92
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
Create New Project contrib and store project configuration #2899
Conversation
Using this new contribution, we can wait for workbench initialization and run the project initialization tasks.
configuration id: `positron.useProjectWizard`
Add the flag to the Welcome page check as well.
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.
Overall LGTM!
I had a few philisophical questions about the feature flag + dev mode switching and comments.
One thing I noticed in looking at the code was that some of the useState()
s were doing extra computation. The code wasn't from this PR though so I couldn't figure out how to comment on it.
In pythonEnvironmentStep.tsx
for the const [interpreterEntries, setInterpreterEntries]...
example it will rerun the getPythonInterpreterEntries()
every re-render. (You can verify by putting a debug or console.count()
at the top of that function. If you just switch to adding a lambda in front you avoid this.
// TODO: [New Project] Remove feature flag when New Project action is ready for release | ||
// This removes the action in the New menu, the application bar File menu, and the command | ||
// palette when the feature flag is not enabled. | ||
const projectWizardEnabled = ContextKeyExpr.deserialize(`config.${USE_POSITRON_PROJECT_WIZARD_CONFIG_KEY}`); |
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.
Does this mean we won't be able to turn off the wizard? I wonder if it would be better to just switch the default value for the flag to true
? (Obviously not applicable to this PR though.)
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.
Currently, the wizard is enabled by default in dev builds and not available at all in release builds. To allow for testing of the WIP wizard in release builds while not making the wizard accessible by default (since functionality is incomplete), the config flag allows us to manually enable the wizard in release builds. In other words, the new flagging logic maintains that the wizard can't be switched off in dev builds, but can now be toggled on/off in release builds (off by default).
Once the wizard is ready for general use, the config flag and dev environment checks will be removed, and the wizard will be available in all the applicable menus.
properties: { | ||
[USE_POSITRON_PROJECT_WIZARD_CONFIG_KEY]: { | ||
type: 'boolean', | ||
default: false, |
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.
Could this be set to default with the development status to simplify the logic in other places? Then when things are good to go you'd just need to switch this default value instead of modifying the logic elsewhere?
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'd like to do this as it would make toggling the switch much cleaner! I might be blocked by when IsDevelopmentContext
is available though.
I tried to use the development status as the default here, but I wasn't sure how to access IsDevelopmentContext
without using the ContextKeyService
, which I'd need to pass via the PositronNewProjectContribution
constructor and wrap this config flag registration call in a function. This delays the registration of the config option, which is okay for the main entrypoints to the wizard, but seems to be too late for the Welcome page (need to trigger a re-render for the config setting to be reflected).
I think IsDevelopmentContext
isn't set until the ContextKeyService
is initialized, which is why reading IsDevelopmentContext
in top-level code didn't yield the expected result.
proj_wizard_config.mp4
src/vs/workbench/browser/positronNewProjectWizard/components/steps/pythonEnvironmentStep.tsx
Show resolved
Hide resolved
src/vs/workbench/browser/positronNewProjectWizard/components/steps/pythonEnvironmentStep.tsx
Show resolved
Hide resolved
Although envType is irrelevant for the Existing Environment env setup type, clearing it from the config means that if the user chooses New Environment after having chosen Existing Environment, the envType will be cleared and the user will have to re-select it. The envType can be cleared if applicable when the project config is being constructed.
Some of the `useState`s in pythonEnvironmentStep involve function calls that re-computed on every render. To avoid the unnecessary re-computation, wrap the initial values in arrow functions.
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.
@nstrayer Thank you for the useState
extra computation tip! This is now in my React cheatsheet :)
I've modified a couple of the useState
s in the pythonEnvironmentStep.tsx to use arrow functions to avoid the re-evaluation on each render.
// TODO: [New Project] Remove feature flag when New Project action is ready for release | ||
// This removes the action in the New menu, the application bar File menu, and the command | ||
// palette when the feature flag is not enabled. | ||
const projectWizardEnabled = ContextKeyExpr.deserialize(`config.${USE_POSITRON_PROJECT_WIZARD_CONFIG_KEY}`); |
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.
Currently, the wizard is enabled by default in dev builds and not available at all in release builds. To allow for testing of the WIP wizard in release builds while not making the wizard accessible by default (since functionality is incomplete), the config flag allows us to manually enable the wizard in release builds. In other words, the new flagging logic maintains that the wizard can't be switched off in dev builds, but can now be toggled on/off in release builds (off by default).
Once the wizard is ready for general use, the config flag and dev environment checks will be removed, and the wizard will be available in all the applicable menus.
properties: { | ||
[USE_POSITRON_PROJECT_WIZARD_CONFIG_KEY]: { | ||
type: 'boolean', | ||
default: false, |
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'd like to do this as it would make toggling the switch much cleaner! I might be blocked by when IsDevelopmentContext
is available though.
I tried to use the development status as the default here, but I wasn't sure how to access IsDevelopmentContext
without using the ContextKeyService
, which I'd need to pass via the PositronNewProjectContribution
constructor and wrap this config flag registration call in a function. This delays the registration of the config option, which is okay for the main entrypoints to the wizard, but seems to be too late for the Welcome page (need to trigger a re-render for the config setting to be reflected).
I think IsDevelopmentContext
isn't set until the ContextKeyService
is initialized, which is why reading IsDevelopmentContext
in top-level code didn't yield the expected result.
proj_wizard_config.mp4
Thank you for the review! I'll merge this and see if I can simplify the flagging as I build out the contribution/service. |
Intent
Summary
Config Setting Preview
Approach
StorageScope.APPLICATION
so any workspace can check to see if it is the new project workspace andStorageTarget.MACHINE
since the new project is being created on the machinepositron.projectWizardEnabled
)IsDevelopmentContext
checks to also check this config settingNext, I'll be leveraging the stored info in the NewProject contribution and service to initialize the project.
QA Notes
The project wizard should be accessible in release builds by enabling the
positron.projectWizardEnabled
setting.