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

Enable only if prettier is installed #199

Closed
jantimon opened this issue Aug 21, 2017 · 42 comments · Fixed by #283
Closed

Enable only if prettier is installed #199

jantimon opened this issue Aug 21, 2017 · 42 comments · Fixed by #283
Labels
enhancement locked Please open a new issue and fill out the template instead of commenting.

Comments

@jantimon
Copy link

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

@CiGit
Copy link
Member

CiGit commented Aug 21, 2017

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

  • Disable the extension for the workspace.
  • Empty all arrays from prettier.{LANGUAGE}Enable settings.

@nilshartmann
Copy link

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
Copy link
Contributor

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

@CiGit
Copy link
Member

CiGit commented Aug 28, 2017

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
Copy link
Author

jantimon commented Aug 28, 2017

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
Copy link
Contributor

RobinMalfait commented Aug 28, 2017

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
Copy link
Author

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
Copy link
Contributor

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
Copy link

I would rather an option like this:
prettier.disableUnlessPrettierConfigIsPresent

@CiGit
Copy link
Member

CiGit commented Aug 30, 2017

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
Copy link

jthegedus commented Aug 30, 2017

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
Copy link
Member

CiGit commented Aug 30, 2017

@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
Copy link

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
Copy link
Member

CiGit commented Aug 30, 2017

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
Copy link

jthegedus commented Aug 30, 2017

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
Copy link

jthegedus commented Sep 5, 2017

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 prettier/prettier-atom#256 (comment)

@jantimon
Copy link
Author

jantimon commented Sep 6, 2017

@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
Copy link

jthegedus commented Sep 6, 2017

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:

  • config files

  • toggle - for using default settings or nothing

  • default settings - prettier installed in pkg.json OR toggle set to default settings

  • nothing - prettier not installed in pkg.json OR toggle set to nothing

  • user level config file - this would be where people define the settings used for default settings above. This could be implemented with either a path to the file or somehow be managed by VSCode.

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
Copy link

robwise commented Sep 14, 2017

@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: prettier/prettier#2760

@CiGit
Copy link
Member

CiGit commented Sep 18, 2017

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.

  • Not persisted files
  • Out of a project files. Personal scripts, ...

@robwise
Copy link

robwise commented Sep 20, 2017

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
Copy link

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
Copy link

@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
Copy link

robwise commented Oct 4, 2017

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
Copy link

ah, thanks, @robwise
my apologies @timramsier

@timramsier
Copy link

@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
Copy link

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
Copy link

Really like the way how prettier-atom handles this.

@gaastonsr
Copy link

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
Copy link

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
Copy link

jcrben commented Nov 7, 2017

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
Copy link
Member

CiGit commented Nov 7, 2017

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
Copy link

duro commented Nov 16, 2017

@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
Copy link
Member

CiGit commented Nov 17, 2017

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
Copy link
Member

azz commented Nov 22, 2017

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

@CiGit
Copy link
Member

CiGit commented Nov 22, 2017

@azz I see some troubles with that.

  • What if it's a sub-dependency ?
  • What if it's hoisted ?

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

  • Root-project
    • sub-project1
      • prettier
    • sub-project2
      • no-prettier

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

@azz
Copy link
Member

azz commented Nov 22, 2017

Good point, I retract mine.

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

@CiGit
Copy link
Member

CiGit commented Nov 22, 2017

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
Copy link

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
Copy link
Member

azz commented Nov 22, 2017

Works for me!

@felixfbecker
Copy link
Contributor

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
Copy link

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.

@github-actions github-actions bot added the locked Please open a new issue and fill out the template instead of commenting. label Apr 13, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement locked Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging a pull request may close this issue.