prettier / prettier-vscode

Visual Studio Code extension for Prettier
https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode
MIT License
5.04k stars 446 forks source link

Enable only if prettier is installed #199

Closed jantimon closed 6 years ago

jantimon commented 6 years ago

Would it be possible to add an option which disables this extension if prettier is not installed in the root package json?

CiGit commented 6 years ago

There are already some ways to disable the extension. I'm not sure one more would really bring something.

nilshartmann commented 6 years ago

I'd also really like to have this option: As I'm using prettier only in very few of my workspaces, some "opt-in behaviour" would be much smoother for my work.

RobinMalfait commented 6 years ago

if you are only using it in very few projects/workspaces. Then you can disable it by default, and enable it where necessary.

CiGit commented 6 years ago

In my setup I can't enable for a given workspace. It is grayed out.

But a similar behavior can be accomplished: User settings : prettier.javascriptEnable:[]

workspace settings where needed: prettier.javascriptEnable:["javascript", "javascriptreact"]

jantimon commented 6 years ago

Well - I have like 20 plugins and many open source and internal projects I work on. If I would have to add/remove/configure all 20 for all of my projects I would rather lose productivity. Which sounds funny for a plugin which should add more productivity.

Many awesome linting tools and similar plugins already do that and I guess the technical solution is quite easy to do.

RobinMalfait commented 6 years ago

I'm confused now. You want an option to configure the extension so that it is enabled/disabled. But you don't want to configure the extension because you have too many projects?

If you have an option that enables/disables it, you still have to configure it? Maybe I'm not getting your point here or I am just confused.

jantimon commented 6 years ago

Ah okay sorry for all the confusion :)

My idea is as follows:

  1. install prettier-vscode
  2. activate prettier-vscode for all projects
  3. add only one global config setting like prettier.smartEnable = true which would run prettier only if the active project has a package.json including prettier

This would reduce the amount of project specific configuration for many plugin users.

RobinMalfait commented 6 years ago

I know that @CiGit is not a big fan of options. I don't care that much about all the options. What are your thoughts @CiGit?

Maybe the option can be long and descriptive: prettier.disableWhenThereIsNoLocalPrettierVersionInstalled (Naming is hard, isn't it)

jthegedus commented 6 years ago

I would rather an option like this: prettier.disableUnlessPrettierConfigIsPresent

CiGit commented 6 years ago

I really want to avoid options. And even more when they seem to add something which is already possible. Maintaining options is hard. Conflicting options is a nightmare.

jthegedus commented 6 years ago

As soon as support for the prettier config files is in all the extensions most projects will use that to define project specific styling. Which means adhering to that is ideal. So detecting when a config for prettier exists already needs to be implemented, so why not add a case to not use prettier if the project doesn't have a config for prettier? I recognise it's another option, but surely this lends itself to being the most useful given users would never need to change the extension's settings again.

If you only supported prettier's config files then you could potentially remove all other options from this extension, no?

CiGit commented 6 years ago

@jthegedus proposal makes more sens to me (it's something new). The problem with this is: How do you define a "project" ? What should happen when you format something which has no config?

jthegedus commented 6 years ago

For project I would say do what prettier does

The configuration file will be resolved starting from the location of the file being formatted, and searching up the file tree until a config file is (or isn't) found.

And when there is no config do whatever prettier cli does when run without flags. Prettier has a --no-config flag for that. It just uses the defaults.

CiGit commented 6 years ago

You just described what's already happening.

And when there is no config do whatever prettier cli does when run without flags. Prettier has a --no-config flag for that. It just uses the defaults.

This is not what was asked in this thread. It should be disabled. Edit: Prettier shouldn't run. I will precise my question: Should nothing happen on formatting or should the default formatter kick in ? Maybe something else ?

jthegedus commented 6 years ago

Of course! Sorry, I was distracted.

If config exists, then use it, else check the new setting that applies the default/user global setting or nothing.

For simplicity in use and implementation, I would only support prettier config files. No default or global user setting. But that's me.

Edit: the most popular opinion is surely a flag that does the default formatting or nothing. Then the extension has a single option for users.

jthegedus commented 6 years ago

There has been some progress on this topic in the prettier-atom plugin repo. The conclusion they have come to (and appear to be implementing) is similar to what I was suggesting. My opinions on the implementation aside, it would be easier for users if the experience was the same across the prettier plugin ecosystem.

Here's the particular discussion point https://github.com/prettier/prettier-atom/issues/256#issuecomment-326832342

jantimon commented 6 years ago

@jthegedus exactly that was what I had in mind when I opened the issue. Reduce the amount of configuration per project to zero and use the existing configuration from the prettier config.

jthegedus commented 6 years ago

Sorry for creating so much noise @CiGit .

It seems like prettier-atom is now going to leave most of their settings and add support for the config file. So no saved code there.

Maybe a solution to reduce the codebase here is to support:

I'll mention it here since it's relevant, the question of managing .editorconfig in addition to prettier still exists (it seems like prettier-atom does the heavy lifting here). IMO it should be done in a similar way to https://github.com/prettier/eslint-config-prettier and not managed here, but that's me.

robwise commented 6 years ago

@jthegedus Yeah, we had some back and forth on the prettier-atom implementation. I figured with the prettier config coming out, we could get rid of a huge amount of code (and I actually coded this up completely), but then there was some push back on that idea.

As far as editorconfig, check out this PR: https://github.com/prettier/prettier/pull/2760

CiGit commented 6 years ago

I would still keep editor's settings. I see some use case where you don't want to setup a prettierrc or whatever config file.

robwise commented 6 years ago

FWIW we (prettier-atom) ended up keeping all the options and then merging them with the prettier options from the config file. Just a heads up, we are having a bunch of problems with backward-compatibility, so before merging, I would suggest making sure you can still use the new version of prettier-vscode with old versions of prettier (assuming you allow using local versions, not sure if you do that).

timramsier commented 6 years ago

I support the idea of only having this run on save if prettier is in the dependencies of the package.json. This is a feature that I used with prettier-atom and it saved a lot of headache where I work. By making it an opt-in based on the package.json dependencies ensured that no one on my team accidentally formatted repos that did not have prettier configured yet. When working with a team of 10+ and 30+ repos, making it based on the package.json in the repo ensures that we aren't having to revert code or review code just because someone had prettier turned on by accident.

jokeyrhyme commented 6 years ago

@timramsier I would suggest that it might be better to look for prettier configuration the way the atom plugin does it: by looking for a .prettierrc file or a "prettier" top-level property in package.json

Searching specifically in dependencies (or devDependencies) would rule out those of us with prettier installed globally, or those of us who run it via npx

robwise commented 6 years ago

FWIW we do it both ways:

prettier-atom settings

Also, some have requested that we check node_modules instead of package.json in case prettier is installed in some sort of third-party bootstrapping library similar to how Create React App works, although we haven't coded that as of yet.

jokeyrhyme commented 6 years ago

ah, thanks, @robwise my apologies @timramsier

timramsier commented 6 years ago

@jokeyrhyme I see value in doing it both ways. I think the Prettier config file would allow it to work for most people (including myself).

guidobouman commented 6 years ago

I would also like a plugin that has the option to only enable when a project is using prettier.

This would help a lot when using Prettier on only some projects. Not all projects have Prettier enabled by default. A lot of legacy open source projects use ESLint for their linting without disabling their formatting rules. To make matters worse, Prettier now conflicts with ESLint when ESLint is set up to work with the highly popular default AirBnB config.

Using the advanced settings, and removing all of the javascript context items has multiple downsides:

  1. It cripples the plugin for all other projects.
  2. The setting is unclear in what it does. It's clearly not meant for this setting.
  3. It actually didn't work in my environment, Prettier still formatted the files.

Disabling this on a workspace context also has it's downsides:

  1. It only works per developer per project. [1] Requiring me to think about this setting for each project. That is an overhead I do not want to put on our junior developers.

[1]: We are free to choose our editor of choice. So I'm not going to add extra config files to our repos for VSCode and any other editor out there. Especially not for a setting that makes a lot of sense for a personal developer setup.

Prettier should make things simpler, not harder.

hannupekka commented 6 years ago

Really like the way how prettier-atom handles this.

gaastonsr commented 6 years ago

Took me a while to find the relevant conversations.

I think it should work if the dependency is found in node_modules.

This is the way it works the eslint plugin now. It looks for the dependency in node_modules. Otherwise, it says

Failed to load the ESLint library for the document /Users/jdoe/Code/file.js

To use ESLint please install eslint by running 'npm install eslint' in the workspace folder Code
or globally using 'npm install -g eslint'. You need to reopen the workspace after installing eslint.

Alternatively you can disable ESLint for the workspace folder GreenRings-API by executing the 'Disable ESLint' command.

This is the relevant conversation in the create-react-app repository. Before that, dependencies needed to be installed globally. Or needed to be listed locally in your dependencies. Now no, it is automatically picked up from your node_modules.

gaastonsr commented 6 years ago

Oh, the eslint plugin also requires to have a config file. Otherwise, you get

No ESLint configuration (e.g .eslintrc) found for file: /Users/gsanchez/Code/GreenRings-API/bookshelf.js
File will not be validated. Consider running 'eslint --init' in the workspace folder GreenRings-API
Alternatively you can disable ESLint by executing the 'Disable ESLint' command.
jcrben commented 6 years ago

Are the maintainers open to a PR implementing something similar to the prettier-atom approach?

I'm currently using the workaround to empty out the arrays globally and enabling on a per project basis but I'd rather have it happen more automatically based on my config.

CiGit commented 6 years ago

PR would be welcome. I really think searching for a config only would be enough. (package.json's "prettier" key included) ie prettier.resolveConfig !== null but this can be somewhat tricky with multi-root ( #243 ). I've no idea how to resolve this correctly... There is also prettier/prettier#2760 which may change resolveConfig's return

duro commented 6 years ago

@jcrben and @CiGit we need this now that VSCode support multi-root workspaces, and the existing workaround does not seem to be supported at the "Folder" config level.

I have a multi-root workspace where one of the root projects uses prettier, and the other does not. And when I try the workaround in the folder specific vscode config I get the following warning:

"This setting cannot be applied now. It will be applied when you open this folder directly."

Seems we need this feature more than ever.

CiGit commented 6 years ago

We have a PR which handles multi-root #252, but it won't help for that case.

I may find some times in next days to come up with a quick and NotSoSatisfying PR. ie: noop when no config file found. @jcrben May have an other better idea tho.

azz commented 6 years ago

I think I like the "only format if prettier is in ${rootDir}/node_modules" approach (behind a setting)

CiGit commented 6 years ago

@azz I see some troubles with that.

This approach is all or nothing. You can't open your project as a sub-project:

Advantage, we could remove it during extension initialization and don't implement it as a noop. (again multi-root ...)

azz commented 6 years ago

Good point, I retract mine.

package.json search up the directory tree from the file being formatted up to the root directory?

CiGit commented 6 years ago

This could be a thing. But it limits to node projects. I still prefer relying on a prettier config which you can put in every projects. And if this is true: resolveConfig(File) === null -> throw new Error("Skip me!")

gaastonsr commented 6 years ago

I like the idea of having a config file. I just don't like the idea of relying on a package.json.

Because if we use something like create-react-app, prettier won't be listed in the dependencies but it will be available in node_modules.

azz commented 6 years ago

Works for me!

felixfbecker commented 6 years ago

Whether it's a setting prettier.enable or only running when prettier is installed in node_modules, we desperately need a way to opt-in into prettier per-folder. Currently there are only settings to disable prettier (which is cumbersome), but the majority of repositories (open source, in a company, etc) will not use prettier and you don't want to reformat every file automatically in these repositories. You only want to format files if the repository actually uses prettier for their code style.

github-actions[bot] commented 4 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.