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 prettier.forceFormatDocument to code actions #2500

Closed
wants to merge 4 commits into from
Closed

Add prettier.forceFormatDocument to code actions #2500

wants to merge 4 commits into from

Conversation

roottool
Copy link
Contributor

@roottool roottool commented Apr 3, 2022

  • Run tests
  • Update the CHANGELOG.md with a summary of your changes

Resolves #1555

Overview

This PR adds two changes

  • Add prettier.forceFormatDocument to code actions
  • Adapt settings.json to this code action addition

Adapt settings.json to this code action addition

I reset editor.defaultFormatter and turn off editor.formatOnSave. Because this extension is able to format by source.prettier.forceFormatDocument in editor.codeActionsOnSave without the two items.

Run tests screenshot

Run tests screenshot

feat: Add `forceFormatDocument` in code actions #1555

docs: Add a change by this PR #1555
@wedi
Copy link

wedi commented Apr 14, 2022

@roottool Thank you for taking the initiative to get this long awaited feature in. 🎉

Is there a special reason why you chose to name the action source.prettier.forceFormatDocument? I am asking because it looks like there is a de facto standard to use fixAll.<productname>. This is used in Microsoft's own eslint extension and stylelint mentions in its docs that you can enable all auto fixing by setting "source.fixAll": true.

@roottool
Copy link
Contributor Author

@wedi I forgot that I wrote the reason, sorry. It has two reason.

First, I wanted to be able to explicitly specify the order of execution. Because I don't know the execution order within "source.fixAll": true.
A codeaction that is added by this PR must be able to run from Prettier to AutoFix by ESLint in that order.

The above solution runs the default formatter (which can be set to Prettier) then uses the code action for eslint afterwards.
#1555

Second, I have decided that Source is more appropriate. Because I have determined that a statement ignoring the formatter's settings is not equivalent to a linter error.

Fix all actions automatically fix errors that have a clear fix that do not require user input.
VS Code API Docs - SourceFixAll

And this comment encouraged me to use Source.

For exposing actions for the CodeActionsOnSave option, the plugin needs to provide a Code Action by using the CodeAction API, and especially a CodeActionKind of type "source"
#1555 (comment)

FYI: This codeaction names were appropriated from the existing command name. Since this codeaction uses that the existing command.

"ext.command.forceFormatDocument.title": "Format Document (Forced)",

If you have a good name for it, I'd be glad to hear it!

@wedi
Copy link

wedi commented Apr 21, 2022

@roottool using the standard naming and choosing a custom execution order is not mutually exclusive. You can customize the order of execution by using the full names like this:

settings.json:

  "editor.codeActionsOnSave": [
    "source.organizeImports",
    "source.fixAll.eslint",
    "source.fixAll.stylelint"
  ],

As prettier fixes all source [code] styling issues, source.fixAll.prettier sounds reasonable to me but I am not further involved in this project. I just wanted to point out that there is a naming convention which you did not follow.

Personally, I'll be happy with whatever name gets chosen and I'm exited to see this merged. :)

nujhong added a commit to kinetics-dev/kinetics that referenced this pull request May 13, 2022
Using `eslint-plugin-prettier` is slower than running prettier itself,
the recommended approach is to run prettier first then follow by eslint
to fix all the problems. VScode integration will be resolved once this
PR gets merged prettier/prettier-vscode#2500.
@@ -1,10 +1,10 @@
// Place your settings in this file to overwrite default and user settings.
{
"editor.codeActionsOnSave": {
"source.prettier.forceFormatDocument": true,
Copy link
Member

Choose a reason for hiding this comment

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

Is this a new recommended best practice from the VS Code team or just preference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not. I'm going to undo this difference if it is not necessary.

This difference has two intent.
First, The intent of this difference is to resolve #1277 by realizing the following quoted portion.

Originally posted by @rohit-gohri in #1277 (comment) is a working solution for running prettier before eslint.

The above solution runs the default formatter (which can be set to Prettier) then uses the code action for eslint afterwards.
#1555 (comment)

Second, it is for testing the created new code action in this PR.
The intention was to make it clear that I was testing through the code action.

Choose a reason for hiding this comment

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

It looks like Microsoft's recommended settings

Although you can also use the formatter on save using the setting editor.formatOnSave it is recommended to use the editor.codeActionsOnSave feature since it allows for better configurability.

https://github.com/microsoft/vscode-eslint

const FORCE_FORMAT_DOCUMENT_CODE_ACTION_KIND = CodeActionKind.Source.append(
FORCE_FORMAT_DOCUMENT_COMMAND
);
const FORCE_FORMAT_DOCUMENT_TITLE = "Format Document (Forced)";
Copy link
Member

Choose a reason for hiding this comment

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

Why is it forced?

Copy link
Contributor Author

@roottool roottool Jun 24, 2022

Choose a reason for hiding this comment

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

Because this code action uses a command of the same name that exists in the command palette.
But I would be willing to adopt any better title. 👍
For example,

Suggested change
const FORCE_FORMAT_DOCUMENT_TITLE = "Format Document (Forced)";
const FORMAT_DOCUMENT_TITLE = "Prettier: Format Document";

Could a code action be added for formatting? Such that if a user were to hit ctrl+shift+p in VS Code it would show something like "Prettier: Format Document"
#1555 (comment)

Copy link
Member

@ntotten ntotten left a comment

Choose a reason for hiding this comment

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

A few comments

@roottool
Copy link
Contributor Author

I replied to a few comments. But I'm sorry that it took me so long to reply.

This commit undo two things.

- The previous incorrect commit
- The `yarn prettier` execution by lint-staged
  This difference is not focused in this PR.
@ntotten
Copy link
Member

ntotten commented Jul 21, 2022

I kind of missed the "forced" part of this PR. I guess I am confused what the point of this is? Why would you want to always force prettier to format files that you have set to be ignored?

@nujhong
Copy link

nujhong commented Jul 25, 2022

I think the contributor was re-using an existing command in the project as he mentioned https://github.com/prettier/prettier-vscode/blame/main/package.nls.json#L3

If I understood it correctly, the original commit dcc531f was aimed to provide a way to "Format Document (Forced)" which means 👉 format document specifically with formatter esbenp.prettier-vscod

Therefore, the goal of this PR was to re-use the same command so that it can be registered as a CodeAction and allowing users to specify the order of code actions by preference.

This ultimately solves the good old "Run Prettier First, then Run ESLint" problem as an alternative approach to doing it via CLI commands or eslint-plugin-prettier.

Screen Shot 2022-07-25 at 9 45 24 pm

Looking forward to this being merged!

@roottool
Copy link
Contributor Author

@nujhong Thank you for the explanation assistance!

@ntotten This CodeAction name includes "forced" is because I uniformed the name at an existing command name. The CodeAction re-uses "Format Document (Forced)" command already in this extension.

"ext.command.forceFormatDocument.title": "Format Document (Forced)",

Including the appropriation of this name, the implementation intent is as he commented.
I have no intention of enforcing the format on me. I just appropriated the name.

@nd0ut
Copy link

nd0ut commented Dec 28, 2022

Any updates?

@ntotten
Copy link
Member

ntotten commented Jan 11, 2023

I am going to close this. This PR doesn't work and it doesn't have tests. If somebody is interested in contributing this please continue the discussion here: #1555

@ntotten ntotten closed this Jan 11, 2023
@roottool roottool deleted the feature/1555/add-code-action branch January 11, 2023 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants