Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Support Prettier 1.6 Configurations #256

Closed
k15a opened this issue Aug 28, 2017 · 15 comments
Closed

Support Prettier 1.6 Configurations #256

k15a opened this issue Aug 28, 2017 · 15 comments
Assignees

Comments

@k15a
Copy link

k15a commented Aug 28, 2017

Prettier 1.6 got released some hours ago and you can now configure Prettier per project. It would be great to support the configuration options with the Atom editor plugin.

In my mind the perfect use case would be:

  • use the configuration from the editor plugin to display code in Atom
  • use the configuration from the project to save the code

Why?

  • we can edit the code with our preferred style
  • git diff will show a meaningful diff

If that's not possible it would still be great to support the project specific settings and .prettierignore.

What do you think?

@darahak
Copy link
Collaborator

darahak commented Aug 28, 2017

In my mind the perfect use case would be:

  • use the configuration from the editor plugin to display code in Atom
  • use the configuration from the project to save the code

@k15a That's an interesting take, but I don't think it would be a desirable behavior for most people (could be wrong though).
Personally, I would assume the config files prevail no matter what, if they are detected.

@darahak
Copy link
Collaborator

darahak commented Aug 28, 2017

@robwise FYI there are a few regressions being reported on 1.6.0.
We can start implementing this, but should wait for these issues to get patched before releasing anything.

@robwise
Copy link
Collaborator

robwise commented Aug 28, 2017

@darahak Roger that, will probably take me a little to get to this. We need to think about who wins if there is a conflict. Here's an example of what I mean (I bolded the one I think would win in the given scenario):

  • prettier-atom sets tabs, prettier config file sets spaces
  • editorconfig sets tabs, prettier config file sets spaces
  • eslint config sets tabs, prettier config file sets spaces
  • prettier-atom detects that file is json so we use json parser, prettier config says to use babylon parser

@robwise robwise self-assigned this Aug 30, 2017
@robwise
Copy link
Collaborator

robwise commented Sep 3, 2017

I don't know, now that I'm looking into it, I think it doesn't make sense for us to replicate all of the code that prettier already has inside of it when trying to determine how to format a file. We should just defer to the config if it exists.

Here is what I'm proposing we do:

  • detect whether or not the file is ignored by a prettierignore (don't format if so)
  • have a new option in prettier-atom settings like "don't format on save if no prettier config is found". If this is checked, we detect whether or not there is a prettier config (.prettierrc file, prettier.config.js file, or a "prettier" key in your package.json file), and don't format if no such file is found
  • Otherwise, format with prettier (letting prettier infer the options from the config, or if there is no config, let it just use defaults).

Then we'd just completely remove custom prettier settings from the Atom package's options (as well as the editorconfig integration and the "auto" tab width inference)? If you want to configure prettier, you should do it with a prettier config file and not the Atom package?

We could also conceivably get rid of globs as well, we could just rely on the don't format on save if no prettier config is found and the prettierignore files?

@darahak
Copy link
Collaborator

darahak commented Sep 3, 2017

@robwise Sounds good. Not only because we avoid replicating code, but because it's what the users would expect.

@robwise
Copy link
Collaborator

robwise commented Sep 4, 2017

This is what the new settings pane could look like, obviously we are getting rid of a huge amount of options now that we expect you to handle things from prettierrc and prettierignore files:

This might be controversial or it might not, I'm not sure. But my argument is that this would help to alleviate that flood of issues that turn out to be a misunderstanding of the complex logic going on.

robwise added a commit that referenced this issue Sep 4, 2017
… settings where possible

BREAKING CHANGE: Blacklist globs, whitelist globs, formatting options, parser-specific scopes, and
editorconfig integration have all been removed. You can replace this functionality completely by
adding a [Prettier configuration file](https://github.com/prettier/prettier#configuration-file)
and/or a `.prettierignore` file.

Resolves #256
@robwise
Copy link
Collaborator

robwise commented Sep 4, 2017

^ I made a preliminary PR that implements what I outlined. If anyone reading this could use that branch in development mode for a while as they go about their day programming, it would be greatly appreciated. This way we can find any bugs and determine if we eliminated necessary functionality before we push it to everybody.

@darahak
Copy link
Collaborator

darahak commented Sep 4, 2017

@robwise Was getting rid of all Atom settings part of your initial proposal? I don't remember 😕
My PR review is contradicting my last post here, but maybe I read too fast and totally missed the point.

@robwise
Copy link
Collaborator

robwise commented Sep 5, 2017

Oh, I edited it at some point to be more specific about what I meant, sorry that might have been after you replied. But no worries, that's why I didn't merge the PR or anything because I wasn't sure this was really the best way to go. I will add that other stuff back in.

@pascalduez
Copy link

Hi,

how would that work between the ESLint integration and the config file then?
For instance, I would like it to follow almost all of our ESLint config (Airbnb), except the printWidth which I would like to force back to 80.

@bhardy
Copy link

bhardy commented Sep 11, 2017

I just wanted to pipe in and say I would really appreciate this feature. The one compromise we had to make to get everyone working with prettier at work was to switch the Print Width to 100. That means when I switch between personal projects and work I need to open up the package settings and manually toggle it. (If anyone has a better workflow for me please message me! haha)

@robwise
Copy link
Collaborator

robwise commented Sep 11, 2017

how would that work between the ESLint integration and the config file then?
For instance, I would like it to follow almost all of our ESLint config (Airbnb), except the printWidth which I would like to force back to 80.

I would just override that rule in your eslintconfig and you wouldn't really need a prettier config

I just wanted to pipe in and say I would really appreciate this feature. The one compromise we had to make to get everyone working with prettier at work was to switch the Print Width to 100. That means when I switch between personal projects and work I need to open up the package settings and manually toggle it. (If anyone has a better workflow for me please message me! haha)

We're working on it but I'm waiting on a new version of prettier to be released because the current version is async and I can't use async with Atom's format on save.

@pascalduez
Copy link

I would just override that rule in your eslintconfig and you wouldn't really need a prettier config

Having a greater value in the ESLint config is desired to bring more flexibility, for instance long lines that can't break properly, that way you don't have to use disable comments all the time.
But as for Prettier, it makes a big difference, 100 chars lines are really too long. And it will apply to all lines.

Wouldn't it be possible to make the Prettier config prevail on anything?

@robwise
Copy link
Collaborator

robwise commented Sep 12, 2017

You'd have to make an issue/PR on prettier-eslint as that is handled under the hood by that tool, we just call out to it from prettier-atom and it reads the eslint config itself.

@robwise
Copy link
Collaborator

robwise commented Sep 16, 2017

Implemented in #261

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants