Implement Prettier Config #261

robwise · 2017-09-04T03:25:58Z

No description provided.

MrLoh · 2017-09-04T18:34:27Z

can't wait for this, thanks a lot for the hard work

darahak · 2017-09-04T19:58:38Z

We should consider commiting transpiled files separately. It's a bit difficult to navigate and review big changes.
We can update the contributing guides accordingly.

darahak

I think we should keep formatting options and EditorConfig support (see comments).

For the rest, it totally makes sense to drop them in favor of the new config file.

I didn't look at the code specifically. I think the tests should be reliable enough for this kind of refactoring.

darahak · 2017-09-04T20:13:32Z

src/formatOnSave/isPrettierInPackageJson.js

@@ -1,40 +0,0 @@
-// @flow


I might have misunderstood, but isn't this the feature that lets the package use local Prettier installs instead of the packaged one? I thought it was useful for a lot of devs.

This is the one that determines whether or not to run formatOnSave based on whether prettier is installed, if the user has chosen the option. The code that gets the local instance is still in (that's getPrettierInstance).

darahak · 2017-09-04T20:32:48Z

src/executePrettier/executePrettierOnBufferRange.js

 const handleError = require('./handleError');

 const executePrettier = (editor: TextEditor, text: string) =>
-  getPrettierInstance(editor).format(text, buildPrettierOptions(editor));
+  getPrettierInstance(editor)


I totally missed that point when you described it in #256

I feel like "use a config file or the defaults" is too radical.
We have no fallback if there is no config, and it reduces our coverage of use cases.

I understand this for ESLint because there are a lot of rules, but it's manageable for Prettier.

Consider this:

Not all teams or projects work with config files (I'm in this situation).

The package will not always be used in a project setting: I could hack on a single file without having to define a .prettierrc for example.

darahak · 2017-09-04T20:38:30Z

src/config-schema.json

    "description":
-      "Use [EditorConfig](http://editorconfig.org/) configuration to load project-based settings for Prettier formatting.<br>Takes precedence over your settings. The following settings will be overriden if they are found in your `.editorconfig`: **Use tabs**, **Tab width**, **Print width**.",


Maybe we should keep this option? At least until prettier/prettier#42 is closed.
It's the third most popular request if you sort issues by 👍 reactions.

And then do a reverse merge, like if it's in the prettier config's options, use that, if not, use the editor config's options?

Yes, basically keep it as a fallback for people who don't use config files.

robwise · 2017-09-05T02:52:34Z

@darahak So you think:

keep:

formatting options
editorconfig inference

remove:

blacklist globs
whitelist globs
ability to not format on save if prettier is not installed locally
parser-specific scopes (or maybe we still need this? — will investigate)

darahak · 2017-09-05T09:43:04Z

@robwise Yes, basically keep formatting options because they are still a good interface for CLI single options, and EditorConfig support because Prettier Config is not a complete replacement IMO (see prettier/prettier#42 for related discussion).

I'm not sure about these two:

ability to not format on save if prettier is not installed locally

parser-specific scopes (or maybe we still need this? — will investigate)

In the end, it's just my opinion. It would be great to have some feedback from users.
Maybe do a beta or a pre-release?

darahak · 2017-09-05T09:58:16Z

On second thought, would it be a good idea to add an option that says: only use Prettier Config or default options (current proposal)?

If it's disabled, the package is able to fall back on user settings.

darahak · 2017-09-06T20:37:51Z

Current PR for EditorConfig support: prettier/prettier#2760

robwise · 2017-09-08T22:09:02Z

Just an update, still working on this, but I have to wait until Prettier releases resolveConfig.sync as otherwise Atom's save doesn't get blocked and we format too late in some cases (also needs the release from prettier-eslint that reverts back to synchronous as well: prettier/prettier-eslint#123)

codecov-io · 2017-09-13T23:13:31Z

Codecov Report

Merging #261 into master will increase coverage by 1.51%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #261      +/-   ##
=========================================
+ Coverage   76.29%   77.8%   +1.51%     
=========================================
  Files          26      29       +3     
  Lines         426     455      +29     
  Branches       40      44       +4     
=========================================
+ Hits          325     354      +29     
  Misses         87      87              
  Partials       14      14

Impacted Files	Coverage Δ
src/atomInterface/index.js	`91.11% <100%> (+0.2%)`	⬆️
src/helpers/atomRelated.js	`100% <100%> (ø)`
...rc/executePrettier/executePrettierOnBufferRange.js	`100% <100%> (ø)`	⬆️
src/helpers/index.js	`100% <100%> (ø)`	⬆️
src/formatOnSave/isFilePathPrettierIgnored.js	`100% <100%> (ø)`
src/executePrettier/buildPrettierOptions.js	`100% <100%> (ø)`	⬆️
src/helpers/general.js	`100% <100%> (ø)`
src/formatOnSave/shouldFormatOnSave.js	`100% <100%> (ø)`	⬆️
src/helpers/getPrettierInstance.js	`100% <100%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cac91ab...2d68a4e. Read the comment docs.

azz · 2017-09-14T01:10:40Z

Tracking for 1.6.2 which will include .sync() prettier/prettier#2804

…g is not present

darahak reviewed Sep 4, 2017

View reviewed changes

jthegedus mentioned this pull request Sep 6, 2017

Enable only if prettier is installed prettier/prettier-vscode#199

Closed

robwise force-pushed the rob/prettier-config branch from 7e14b67 to 1adfed0 Compare September 13, 2017 23:11

robwise force-pushed the rob/prettier-config branch 2 times, most recently from 9d0b326 to 0c07a43 Compare September 15, 2017 01:19

robwise added 5 commits September 14, 2017 21:21

feat(prettier): bump bundled version to 1.7.0

20281be

feat(prettier-eslint): bump bundled version to 8.1.0

142de95

feat(prettier-config): use options from prettier-config if present

881bd09

feat(settings): add ability not to format on save if a prettier confi…

97bdf8e

…g is not present

docs(settings): add note about needing a rule in your prettier config

2d68a4e

robwise force-pushed the rob/prettier-config branch from 86e1a22 to 2d68a4e Compare September 15, 2017 01:22

robwise changed the title ~~[WIP] Implement Prettier Config~~ Implement Prettier Config Sep 15, 2017

robwise merged commit 46f32fb into master Sep 15, 2017

robwise deleted the rob/prettier-config branch September 15, 2017 01:34

robwise mentioned this pull request Sep 16, 2017

Support Prettier 1.6 Configurations #256

Closed

robwise mentioned this pull request Mar 20, 2018

Plugin support? #395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Prettier Config #261

Implement Prettier Config #261

robwise commented Sep 4, 2017

MrLoh commented Sep 4, 2017

darahak commented Sep 4, 2017

darahak left a comment •

edited

Loading

darahak Sep 4, 2017

robwise Sep 5, 2017

darahak Sep 4, 2017

darahak Sep 4, 2017

robwise Sep 5, 2017

darahak Sep 5, 2017

robwise commented Sep 5, 2017

darahak commented Sep 5, 2017

darahak commented Sep 5, 2017

darahak commented Sep 6, 2017

robwise commented Sep 8, 2017

codecov-io commented Sep 13, 2017 •

edited

Loading

azz commented Sep 14, 2017

		"description":
		"Use [EditorConfig](http://editorconfig.org/) configuration to load project-based settings for Prettier formatting.<br>Takes precedence over your settings. The following settings will be overriden if they are found in your `.editorconfig`: Use tabs, Tab width, Print width.",

Implement Prettier Config #261

Implement Prettier Config #261

Conversation

robwise commented Sep 4, 2017

MrLoh commented Sep 4, 2017

darahak commented Sep 4, 2017

darahak left a comment • edited Loading

Choose a reason for hiding this comment

darahak Sep 4, 2017

Choose a reason for hiding this comment

robwise Sep 5, 2017

Choose a reason for hiding this comment

darahak Sep 4, 2017

Choose a reason for hiding this comment

darahak Sep 4, 2017

Choose a reason for hiding this comment

robwise Sep 5, 2017

Choose a reason for hiding this comment

darahak Sep 5, 2017

Choose a reason for hiding this comment

robwise commented Sep 5, 2017

darahak commented Sep 5, 2017

darahak commented Sep 5, 2017

darahak commented Sep 6, 2017

robwise commented Sep 8, 2017

codecov-io commented Sep 13, 2017 • edited Loading

Codecov Report

azz commented Sep 14, 2017

darahak left a comment •

edited

Loading

codecov-io commented Sep 13, 2017 •

edited

Loading