Add option to require @prettier or @format pragma by wbinnssmith · Pull Request #2772 · prettier/prettier

wbinnssmith · 2017-09-08T06:25:58Z

Inspired by eslint-plugin-prettier and the discussion in #2397, this implements requiring a special comment pragma to be present in a file's first comment in order to be formatted.

This will help large codebases gradually transition to prettier over time without tons of churn or large code reviews.

I implemented this as a standard prettier "option", not just a typical argv flag, as it is relevant in both the cli and the api. This way it can be provided programmatically, on the command line, or standardized in a prettierrc file so like the style options, every user can use this setting consistently and only apply prettier to relevant files, no mattier their editor integration.

This requires the pragma begin with @ (in fact it's inserted if the user doesn't provide it). Currently the usage implies it must be "prettier" or "format", but it can technically be any value other than "none", which is similar to the trailingCommas option.

cc @vjeux

wbinnssmith · 2017-09-08T06:30:43Z

+ * @flow
+ * @format
+ */
+


hmm.. the whole thing does work. not sure what happened here...

What do you mean? Isn't it supposed to be working?

By the way, it doesn't have to be an integration test. It can be written as a normal test. You can create a new folder and tweak the config in jsfmt.spec.js

azz · 2017-09-08T06:33:15Z

As I mentioned in #2397 (comment), I'm not sure why --require-pragma can't just be a boolean (--no-require-pragma would be the opposite).

wbinnssmith · 2017-09-08T06:49:48Z

cc the eslint-plugin-prettier folks: @not-an-aardvark @zertosh

mitermayer · 2017-09-08T15:15:21Z

If we go through with this let's stick with either @format or @prettier. I would suggest just sticking with @format

vjeux · 2017-09-08T16:27:31Z

+    re = reForPragma[pragma];
+  } else {
+    // @pragma surrounded by any amount of whitespace on either or both sides
+    re = reForPragma[pragma] = new RegExp(`\\s*@${pragma}\\s*`);


Your \s* does absolutely nothing. It matches -- 0 -- or more whitespaces.

You need to escape pragma, otherwise people can start adding .* to match literally anything.

It is also worth thinking about if [email protected] should match the @prettier pragma. Perhaps we should check how Flow checks for @flow and do something similar?

vjeux · 2017-09-08T16:29:03Z

 }

+// Cache compiled regexps in the likely case the same pragma is used for many
+// files.


No seriously, this is not needed. Creating a dynamic regex isn't going to be a bottleneck.

vjeux · 2017-09-08T16:30:03Z

+  // if the user provided a value with `@` in the beginning anyway, strip it off.
+  if (value.startsWith("@")) {
+    return value.substr(1);
+  }


Please don't silently drop things. Instead you want to throw an exception with a clear error message.

Actually, it seems to be confusing so why not require people to add @ in the command line.

--pragma @format

vjeux · 2017-09-08T16:30:47Z

+  const value = argv["require-pragma"];
+
+  if (value === undefined) {
+    return value;


Can you be explicit and return undefined;, otherwise it's not instantly clear that value is undefined

vjeux · 2017-09-08T16:47:38Z

                           Print trailing commas wherever possible when multi-line. Defaults to none.
  --parser <flow|babylon|typescript|postcss|json|graphql>
                           Specify which parse to use. Defaults to babylon.
+  --require-pragma <none|format|prettier>


I would just name it --pragma, it feels weird to have the require- in front.

Given the fact that we want it to be a boolean, disregard my renaming suggestion.

vjeux · 2017-09-08T16:51:39Z

If you address my comments, I'll get it in, thanks for building it!

vjeux · 2017-09-08T16:55:30Z

I somehow skipped @azz comment. I actually really like the idea. What we could do is to hardcode @format and @prettier to be activated via the boolean. It also gives us option value to enable

/* @prettier disable */
/* @prettier trailing-comma=es5 */
...

in the future

wbinnssmith · 2017-09-08T19:01:04Z

@azz @mitermayer @vjeux don't you think we should keep this flexible for compatibility with eslint-plugin-prettier's pragma option allowing arbitrary strings?

I definitely see the advantage of keeping this to a known value so in the future option-values can be applied. I'm still worried about the existing adoption of eslint-plugin-prettier's arbitrary pragmas though.

lydell · 2017-09-08T19:45:31Z

Isn't it super easy to migrate from a custom pragma name, though? Just a simple search-and-replace across the entire repo.

vjeux · 2017-09-08T19:45:48Z

I doubt many people used a different pragma, if they are using the pragma at all.

lydell · 2017-09-08T19:49:31Z

+    re = reForPragma[pragma] = new RegExp(`\\s*@${pragma}\\s*`);
+  }
+
+  const firstComment = astComments[0];


Why does it have to be the first comment? There are lots of magic comments you might start your files with:

@prettier

@flow

eslint-disable no-console

licensing stuff

All the fb magic comments only work if it's the first comment of the file.

it doesn't need to be the first thing in the comment, but it needs to be a part of the first comment, like in a docblock. You can still have other pragmas and licensing information within that first docblock (see most open source FB projects)

There are two practical reasons for this:

We don't need to parse the file, we can just grep until we see */ to find it which helps making it fast.

We use them to toggle syntax variants, so we can't actually parse the file until we know which variant to use.

We're not doing any of those two right now with prettier but those are two optimizations that would be useful.

Good, sounds like this has been thought through then. 👍

Actually, we should probably implement 1), otherwise it's going to throw error for js files that prettier doesn't understand or be super slow for giant generated files where it doesn't need to.

I played around a little bit with Flow, and this works:

/* function notCode() { return 1; } */ // @prettier // something something @flow side code;

It appears as if @flow can be present anywhere in any comment that comes before all code. Additionally, @flow seems to have to be located at the very beginning of a comment or line, at the very end of a line, or surrounded by whitespace.

mitermayer · 2017-09-08T22:59:24Z

 const config = require("./src/resolve-config");
-
-const PRAGMA_RE = /@\bprettier|format\b/;
+const docblock = require("jest-docblock");


My only concern on using jest-docblock for extracting the tags is that it has no documentation or information around it, (https://www.npmjs.com/package/jest-docblock).

If that is a multi-purpose module maybe we should include some documentation around it, otherwise we could either implement our own naive docblock pragma extractor or use other established parser like doctrine.

@cpojer what are your thoughts around it ? is jest-docblock meant to be a private module for jest itself ?

it's already used to do the pragma extraction in eslint-plugin-prettier :) Also see prettier using jest-validate right below in package.json here.

it's so useful I'd be happy to document its api.

I imagine jest split itself as many packages for this very reason?

jest-docblock in particular was split out because it was needed in multiple packages inside of jest - an awesome side-effect is that it can be used separately by other projects!

It has no documentation since I was lazy when it was extracted, so a documentation PR would be awesome

Use it, it's great. No docs necessary, source should be reasonably simple :)

jestjs/jest#4457 :)

vjeux · 2017-09-08T23:01:04Z

jest-docblock is what we use for jest and runs in prod at Facebook so even though it has no documentation, I would trust it. Also, we're pinning a version and it's unlikely to change drastically so we should be good to go.

suchipi · 2017-09-08T23:02:03Z

I'm in favor of a hardcoded pragma; I think it will be better for the community long-term; instead of a user wondering, "what does that mean?" every time they go into a new project, and then realizing, "oh, that's how they configured prettier", a user who has wondered that once will understand what's going on in the next project, when they see the same pragma.

Because of that motivation, I'd prefer a single, hardcoded pragma (like what flow does). My preference is for @prettier over @format, but it doesn't matter too much as long as we pick one.

vjeux · 2017-09-08T23:09:00Z

Facebook entire codebase is using @format because I was hoping that it would become a standard across all languages. So that fb engineers didn't need to know that prettier is what powers formatting.

mitermayer · 2017-09-08T23:24:16Z

I also vote for @format

suchipi · 2017-09-09T00:56:36Z

So that fb engineers didn't need to know that prettier is what powers formatting.

I feel like in open source, this is more bad than good; hiding complexity can confuse people. googling "@prettier" will show prettier. Googling "@format" won't.

vjeux · 2017-09-09T02:34:35Z

@suchipi totally agreed, I think that for open source it makes sense to use and promote @prettier. I think that it's worth just supporting both.

Per prettier/prettier#2772 (comment)

wbinnssmith · 2017-09-13T05:19:33Z

Anything I can do to help move this along @vjeux? Apart from maybe moving the integration test to a unit-style snapshot test, I feel like this is in a pretty good spot right now.

lydell · 2017-09-13T05:20:56Z

Travis says that the tests fail – perhaps you could start by looking at those?

wbinnssmith · 2017-09-13T05:26:15Z

ah yeah, I thought this was due to a few lines not being covered like I saw before, but there's an issue with Node 4 not having [].includes. Thanks for the heads up @lydell

@vjeux

Fixes prettier#2397. Inspired by `eslint-plugin-prettier` and the discussion in prettier#2397, this implements requiring a special comment pragma to be present in a file's first comment in order to be formatted. This will help large codebases gradually transition to prettier over time without tons of churn or large code reviews. I implemented this as a standard prettier "option", not just a typical `argv` flag, as it is relevant in both the cli and the api. This way it can be provided programmatically, on the command line, or standardized in a prettierrc file so like the style options, every user can use this setting consistently and only apply prettier to relevant files, no mattier their editor integration. This requires the pragma begin with `@` (in fact it's inserted if the user doesn't provide it). Currently the usage implies it must be "prettier" or "format", but it can technically be any value other than "none", which is similar to the `trailingCommas` option. cc @vjeux

vjeux · 2017-09-13T16:03:22Z

Thanks!

wbinnssmith · 2017-09-13T18:38:52Z

any time @vjeux 🎉

Per prettier/prettier#2772 (comment)

wbinnssmith mentioned this pull request Sep 8, 2017

Support @prettier pragma comment #2397

Closed

wbinnssmith commented Sep 8, 2017

View reviewed changes

vjeux reviewed Sep 8, 2017

View reviewed changes

lydell reviewed Sep 8, 2017

View reviewed changes

wbinnssmith force-pushed the wbinnssmith/require-pragma branch from 9b39c76 to 375f296 Compare September 8, 2017 22:53

mitermayer reviewed Sep 8, 2017

View reviewed changes

wbinnssmith force-pushed the wbinnssmith/require-pragma branch from 375f296 to d838f18 Compare September 8, 2017 23:05

suchipi added a commit to suchipi/jest that referenced this pull request Sep 10, 2017

Create README.md with documentation and usage

a80131e

Per prettier/prettier#2772 (comment)

suchipi mentioned this pull request Sep 10, 2017

Add README to jest-docblock jestjs/jest#4457

Merged

cpojer pushed a commit to jestjs/jest that referenced this pull request Sep 11, 2017

Create README.md with documentation and usage (#4457)

b621cd2

Per prettier/prettier#2772 (comment)

Will Binns-Smith added 7 commits September 12, 2017 22:42

Don't quote anything in runPrettier; this is usually handled by a shell

af5206f

Make --require-pragma a boolean option

c38f4a8

Use jest-docblock to find pragmas without parsing the ast

4b30902

Clarify docs

1f3ec83

includes -> indexOf

fcf7d9d

Move test out of integration

556529e

wbinnssmith force-pushed the wbinnssmith/require-pragma branch from c7bc25e to 556529e Compare September 13, 2017 05:43

vjeux merged commit d5e5d66 into prettier:master Sep 13, 2017

wbinnssmith deleted the wbinnssmith/require-pragma branch September 13, 2017 18:38

This was referenced Sep 13, 2017

Transition plan for new requirePragma in prettier core prettier/eslint-plugin-prettier#54

Closed

Relax pragma rules prettier/eslint-plugin-prettier#48

Closed

ghost mentioned this pull request Sep 15, 2017

Update prettier from 1.6.1 to 1.7.0 ikatyang/emoji-cheat-sheet#57

Merged

samouri mentioned this pull request Sep 19, 2017

Add insertPragma option #2864

Closed

tabrindle pushed a commit to tabrindle/jest that referenced this pull request Oct 2, 2017

Create README.md with documentation and usage (jestjs#4457)

173fc41

Per prettier/prettier#2772 (comment)

lock Bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019

lock Bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Uh oh!

Conversation

wbinnssmith commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azz commented Sep 8, 2017

Uh oh!

wbinnssmith commented Sep 8, 2017

Uh oh!

mitermayer commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented Sep 8, 2017

Uh oh!

vjeux commented Sep 8, 2017

Uh oh!

wbinnssmith commented Sep 8, 2017

Uh oh!

lydell commented Sep 8, 2017

Uh oh!

vjeux commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbinnssmith Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbinnssmith Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented Sep 8, 2017

Uh oh!

suchipi commented Sep 8, 2017

Uh oh!

vjeux commented Sep 8, 2017

Uh oh!

mitermayer commented Sep 8, 2017

Uh oh!

suchipi commented Sep 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wbinnssmith Sep 8, 2017 •

edited

Loading

wbinnssmith Sep 8, 2017 •

edited

Loading

suchipi commented Sep 9, 2017 •

edited

Loading