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

Allow codeActionsOnSave to trigger for autoSave #190516

Merged
merged 26 commits into from
Sep 14, 2023

Conversation

justschen
Copy link
Contributor

@justschen justschen commented Aug 15, 2023

Addresses #163920

Removing the code block check allows codeActionsOnSave and createCodeActionsOnSave to run on auto save.

Works for all autoSave settings including afterDelay, onFocusChange, and onWindowChange

(Main change was commenting out the check, other changes with the imports at the top are just a result of the autoSave running organize imports on the imports in that file when the setting for "editor.codeActionsOnSave": { "source.organizeImports": true } is on. )

@justschen justschen self-assigned this Aug 15, 2023
@justschen justschen requested a review from mjbvz August 15, 2023 18:32
@justschen
Copy link
Contributor Author

Requesting @mattbierner since you worked on this last - if there's a better way to do this, I can give it a deeper look 👍

@justschen justschen marked this pull request as ready for review August 15, 2023 19:33
@VSCodeTriageBot VSCodeTriageBot added this to the August 2023 milestone Aug 15, 2023
@mjbvz
Copy link
Contributor

mjbvz commented Aug 16, 2023

See #155059 for a discussion around this. I'm concerned that users will accidentally enable it and then be confused by the behavior (such as unused imports getting silently removed after a delay)

Instead of enabling this by default, maybe we could need an additional setting just for code actions that should be run on autosave, or something like:

"editor.codeActionsOnSave": {
    "source.fixAll": { "onAutosave": true}
  }

AFAIK we also don't support running format on autosave. Maybe we could look into enabling that too in a consistent way

@justschen
Copy link
Contributor Author

justschen commented Aug 21, 2023

Format on auto save seems to be diff from code actions since i know editor.formatOnSave exists.

but here's a working demo of what this looks like in the settings file!
Recording 2023-08-21 at 15 00 15

(in the example, I hit ctrl + s, but still works with auto save on delay/focus change/etc)

@justschen
Copy link
Contributor Author

There's a weird issue with applyOnSaveActions where even if there are things in excludedActions, it will still run the action on save because it's also in codeActionsOnSave. To eliminate that, I added filters that remove them from the final function call to applyOnSaveActions.

@justschen
Copy link
Contributor Author

Added the following to match notebook code actions on save settings UI
Recording 2023-08-28 at 12 39 54

@justschen justschen removed this from the August 2023 milestone Aug 30, 2023
@justschen justschen added this to the September 2023 milestone Aug 30, 2023
mjbvz
mjbvz previously approved these changes Sep 7, 2023
const settingsOverrides = { overrideIdentifier: textEditorModel.getLanguageId(), resource: model.resource };
const setting = this.configurationService.getValue<{ [kind: string]: boolean } | string[]>('editor.codeActionsOnSave', settingsOverrides);
// Keeping string | boolean until fully deprecating boolean options
const setting = this.configurationService.getValue<{ [kind: string]: string | boolean } | string[]>('editor.codeActionsOnSave', settingsOverrides);
if (!setting) {
return undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of handling the boolean values below, can you add logic here that converts the old boolean settings to the new enum values: false -> "never", true -> "explict". That should simplify the logic

@justschen
Copy link
Contributor Author

tagging @Yoyokrazy for updates (pushed the code we wrote to this PR)

@justschen justschen enabled auto-merge (squash) September 14, 2023 21:42
@justschen justschen merged commit 153335b into microsoft:main Sep 14, 2023
6 checks passed
@wisscot
Copy link

wisscot commented Sep 27, 2023

Great work, thanks for the fix! Just curious will this go into next release? @justschen

@justschen
Copy link
Contributor Author

@wisscot yes! migrating over from booleans to enums as well in #194345

Keep in mind, a change from this PR is that currently, always only works for cases of explicit save, focus_change autosave and window_change autosave. Code Actions on Auto Save after Delay isn't being supported.

@wisscot
Copy link

wisscot commented Sep 28, 2023

awesome, thanks for the heads up!

@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants