Merge with eslint-plugin-prettify #21

zertosh · 2017-05-15T00:06:14Z

As discussed in #17, eslint-plugin-prettify will merge into eslint-plugin-prettier. eslint-plugin-prettify diffs the original code against the prettier output, and reports fine grained issues, instead of invalidating the entire file.

This PR ports the code at zertosh/eslint-plugin-prettify@2fb5dfb, and adjusts it to conform to @not-an-aardvark's pre-existing code style. A few notes:

The new rule is backwards compatible with the previous one. The option can be an object that is passed to Prettier. In addition to that, it can now be the string "fb" - which sets preferred "Facebook style" (see the README).
A new second option can be passed to the rule, which sets a "pragma" requirement for linting. If it's omitted, then all files are linted. If it's included, then only files with that pragma are linted. eslint-plugin-prettify automatically set a pragma of @format if you used the magic "fb" config. In this port, that's no longer the case (as requested by @kittens).
eslint-plugin-prettify had a special entry no-styles, which added a preprocessor that stripped style rules from the final result. That's not included in this PR, it'll be added in a followup PR.

This code is originally from zertosh/eslint-plugin-prettify@2fb5dfb. It's has been adjusted to conform to the existing ESLint style.

not-an-aardvark

Thanks! This generally looks good, I just have a few small suggestions/questions.

not-an-aardvark · 2017-05-15T00:16:07Z

.eslintrc.js

-}
+// Register ourselves as a plugin to avoid `node_modules` trickery.
+const Plugins = require('eslint/lib/config/plugins');
+Plugins.define('prettier', require('.'));


I think this approach is going to break after eslint/eslint#8465 is merged, which is likely to happen in ESLint 4. (The change makes Plugins an ES6 class rather than a global mutable object, to prevent unrelated modules from conflicting with each other if they both happen to be using eslint in the same process.)

Oh thanks for the heads up! Modifying the node_modules after an install causes yarn issues :/

QQ: We also use this approach at fb because you can't use a path as a plugin. I understand the module resolution complications of supporting relative paths, but would the ESLint team be open to accepting a PR for absolute paths to a plugin?

There's an accepted issue for it at eslint/eslint#6237, but when I looked into it a little while ago it seemed like it could introduce problems even when absolute paths are used. Basically, if I load eslint-plugin-foo with a relative path, and I'm also using a shareable config that references something else called eslint-plugin-foo in node_modules (e.g. another version of the plugin), it wouldn't be possible to configure rules from both plugins independently.

Feel free to comment on that thread if you have any ideas for how to handle that, though.

Hmm, I hadn't realized modifying node_modules breaks yarn. Maybe we should keep Plugins somehow until we have a better way to refer to plugins by path.

Hmm, I hadn't realized modifying node_modules breaks yarn

It causes yarn check to fail. Also, it'll break a new yarn feature I'll be upstreaming soon: "watchman-based check". Basically, you get instant (sub ~100ms) full node_modules verification, which lets use put yarn in front of every js script w/o having to worry about stale modules.

not-an-aardvark · 2017-05-15T00:24:02Z

README.md


-## Migrating to 2.0.0
+ - A string with a pragma that triggers this rule. By default, this rule applies to all files. However, if you set a pragma (this option), only files with that pragma in the heading docblock will be checked. All pragmas must start with `@`. Example:


To me, it seems like it would be better to get this functionality by using normal eslint directive comments, e.g.

/* eslint prettier: ["error", "fb"] */

Is there a reason to have a special pragma for the rule?

tbh, this is a facebookism. We need a to tell our language-agnostic infra that these files conform to a formatter (we use @format). This helps the merge drivers to reconcile changes, and helps other enforcement tooling maintain order. So even if eslint-plugin-prettier didn't understand pragmas, we'd still have to mark these with @format.

not-an-aardvark · 2017-05-15T00:30:04Z

eslint-plugin-prettier.js

+ ret += '\u23ce'; // Return Symbol
+ break;
+ case '\t':
+ ret += '\u21b9'; // Left Arrow To Bar Over Right Arrow To Bar


Nitpick: Maybe this would be more readable if you used the symbols themselves in the code rather than their escape codes. For example:

switch (str[i]) { case ' ': ret += '·'; // Middle Dot, \u00b7 break; case '\n': ret += '⏎'; // Return Symbol, \u23ce case '\t': ret += '↹'; // Left Arrow To Bar Over Right Arrow To Bar, \u21b9 }

That way, if someone sees a symbol in the output, it's easier to tell where in the code the symbol is coming from.

not-an-aardvark · 2017-05-15T00:32:28Z

eslint-plugin-prettier.js

+ // and another's beginning does not have line endings (i.e. issues that occur
+ // on contiguous lines).
+
+ const source = context.getSource();


I think context.getSource() is deprecated -- there doesn't seem to be any documentation for it, and it's in the list of deprecated passthroughs here. Can you replace it with context.getSourceCode().text instead?

not-an-aardvark · 2017-05-15T00:37:47Z

eslint-plugin-prettier.js

+ */
+function reportInsert(context, offset, text) {
+ const pos = getLocFromIndex(context, offset);
+ const range = [null, offset];


I'm not sure null is documented as a valid value in a fix range. Can you replace this with [offset, offset] or something like that instead?

(Admittedly, the design of insertTextAfterRange does make the first value in the range useless, but I don't want this to break if ESLint starts validating ranges later on by asserting that both values are numbers.)

not-an-aardvark · 2017-05-15T00:47:48Z

eslint-plugin-prettier.js

+ if (pragma) {
+ // The pragma is only valid if it is found in a block comment at the
+ // very start of the file.
+ const firstComment = context.getAllComments()[0];


I think context.getAllComments() is also deprecated. Can you use context.getSourceCode().getAllComments() instead?

Also, starting in ESLint 4, shebang comments will be included in the result of sourceCode.getAllComments (see the migration guide). Shebang comments need to be placed at the start of a file, so maybe it would be better to filter them here.

not-an-aardvark · 2017-05-15T00:51:58Z

test/prettier.js

+ code: sections[1],
+ output: sections[2],
+ options: eval(sections[3]), // eslint-disable-line no-eval
+ errors: eval(sections[4]) // eslint-disable-line no-eval


Would it be possible to use JSON.parse here instead? It seems like there's no reason to execute array literals as code.

JSON formatting is too restrictive: Quoting keys, and using double quotes for strings. RuleTester's output is not JSON, so copy/pasting expected output would've required re-formatting. If it's all the same to you, I'd like to keep the eval until it becomes a problem.

not-an-aardvark · 2017-05-15T00:56:44Z

test/prettier.js

+ { code: `/** @format */\n'';\n`, options: ['fb', '@format'] }
+ ],
+ invalid: [
+ '01',


This isn't a strong objection, but I have doubts about whether putting the invalid cases in separate files actually improves debuggability. If a single test case is failing, it seems like it would be easier to search for the corresponding code within a single file rather than opening/closing a bunch of test files to try to find the right one.

Ah, it's actually more about easily looking at a fixture file and easily converting line & column in the test vs the message. It's also very convenient to be able to quickly focus on a few tests by commenting out.

not-an-aardvark · 2017-05-15T00:59:44Z

test/prettier.js

@@ -0,0 +1,92 @@
+#!/usr/bin/env node
+/* eslint-disable node/shebang */


Why is this file executable? RuleTester uses mocha internally, so it seems like this would throw an error if it was just run with node.

It works actually, you just don't get pretty output if there's an error. But I'll remove it since that's an odd use case.

not-an-aardvark · 2017-05-15T01:00:55Z

eslint-plugin-prettier.js

+ // Prettier is expensive to load, so only load it if needed.
+ prettier = require('prettier');
+ }
+ const source = context.getSource();


I think context.getSource() is deprecated -- there doesn't seem to be any documentation for it, and it's in the list of deprecated passthroughs here. Can you replace it with context.getSourceCode().text instead?

vjeux

Looks good to me, @not-an-aardvark do you have any concerns left merging this in?

zertosh · 2017-05-15T18:44:28Z

I enabled "Rebase and merge" to preserve the commits on this PR. If it all looks good, then I can merge and publish.

not-an-aardvark · 2017-05-15T19:15:46Z

By the way, I've been using this script to autogenerate changelogs and git tags, in case we want to keep using it. (It requires a few additional devDependencies that eslint-plugin-prettier currently doesn't have. I'd been running the script locally with a relative path outside of the eslint-plugin-prettier repo, but if we want to keep using it, it might be better to put it in the repo.)

zertosh · 2017-05-16T11:27:39Z

neat script @not-an-aardvark! I just used it and pushed the results :) I'll check it into the repo & add some release instructions. Mind doing an npm release and/or adding me as a collaborator?

not-an-aardvark · 2017-05-16T13:39:53Z

Will do. Are you zertosh on npm?

zertosh · 2017-05-16T13:42:25Z

@not-an-aardvark, yup! Thanks!

not-an-aardvark · 2017-05-16T13:44:34Z

Thanks for confirming. I'm on mobile right now but I'll add you sometime in the next few hours.

not-an-aardvark · 2017-05-16T16:18:44Z

Added you to the npm package.

Summary: `eslint-plugin-prettify` merged into `eslint-plugin-prettier` (prettier/eslint-plugin-prettier#21) Reviewed By: matthewwithanm Differential Revision: D5080518 fbshipit-source-id: 05d568daaa44b189f82ea912b5d1fdedae07f367

zertosh added 4 commits May 14, 2017 15:56

Chore: Use eslint-plugin-prettier without symlink

3e983f1

Chore: Lint .eslintrc.js via CLI automatically

bb912f4

Chore: Update remaining repo links

e313bdd

New: Replace implementation with line-by-line reporting (fixes #17)

3d757e6

This code is originally from zertosh/eslint-plugin-prettify@2fb5dfb. It's has been adjusted to conform to the existing ESLint style.

zertosh requested review from not-an-aardvark and vjeux May 15, 2017 00:06

This was referenced May 15, 2017

Run through prettier zertosh/eslint-plugin-prettify#1

Closed

Improve report location calculations #7

Closed

not-an-aardvark reviewed May 15, 2017

View reviewed changes

zertosh added 5 commits May 14, 2017 22:26

Fix: Use non-global line ending regex

8d9b6e7

Fix: Use unicode literals instead of escapes in showInvisibles

484da18

Fix: Use documented range form when calling insertTextAfterRange

f58b054

Fix: Un-shebang test file

a0c8a0e

Fix: ESLint 4.x compat (handle shebangs and no deprecated APIs)

7ba59f6

vjeux approved these changes May 15, 2017

View reviewed changes

not-an-aardvark approved these changes May 15, 2017

View reviewed changes

zertosh merged commit 6de494f into master May 16, 2017

zertosh deleted the merge-with-prettify branch May 16, 2017 11:16

zertosh mentioned this pull request May 19, 2017

Chore: Add release script #25

Merged

renovate-bot mentioned this pull request May 19, 2017

Update dependency eslint-plugin-prettier to version 2.1.1 renovatebot/renovate#214

Merged

SimenB mentioned this pull request Jan 7, 2018

Add support for eslint-plugin-prettier jfmengels/eslint-rule-documentation#26

Merged

renovate bot mentioned this pull request Oct 9, 2019

chore(deps): update all non-major dependencies juliomrqz/statusfy#387

Merged

1 task

github-actions bot mentioned this pull request Sep 2, 2022

chore: release eslint-plugin-prettier v5.0.0-alpha.0 #517

Merged

5 tasks

github-actions bot mentioned this pull request Mar 10, 2023

chore: release eslint-plugin-prettier (alpha) #553

Merged

github-actions bot mentioned this pull request Jul 6, 2023

chore: release eslint-plugin-prettier (alpha) #566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge with eslint-plugin-prettify #21

Merge with eslint-plugin-prettify #21

zertosh commented May 15, 2017

not-an-aardvark left a comment

not-an-aardvark May 15, 2017

zertosh May 15, 2017 •

edited

Loading

not-an-aardvark May 15, 2017 •

edited

Loading

zertosh May 15, 2017

not-an-aardvark May 15, 2017

zertosh May 15, 2017

not-an-aardvark May 15, 2017

zertosh May 15, 2017

not-an-aardvark May 15, 2017

zertosh May 15, 2017

not-an-aardvark May 15, 2017

zertosh May 15, 2017

not-an-aardvark May 15, 2017

zertosh May 15, 2017

not-an-aardvark May 15, 2017

zertosh May 15, 2017

not-an-aardvark May 15, 2017

zertosh May 15, 2017

not-an-aardvark May 15, 2017

zertosh May 15, 2017

not-an-aardvark May 15, 2017

not-an-aardvark May 15, 2017

zertosh May 15, 2017

vjeux left a comment

zertosh commented May 15, 2017

not-an-aardvark commented May 15, 2017

zertosh commented May 16, 2017

not-an-aardvark commented May 16, 2017

zertosh commented May 16, 2017

not-an-aardvark commented May 16, 2017

not-an-aardvark commented May 16, 2017


		## Migrating to 2.0.0
		- A string with a pragma that triggers this rule. By default, this rule applies to all files. However, if you set a pragma (this option), only files with that pragma in the heading docblock will be checked. All pragmas must start with `@`. Example:

		@@ -0,0 +1,92 @@
		#!/usr/bin/env node
		/* eslint-disable node/shebang */

Merge with eslint-plugin-prettify #21

Merge with eslint-plugin-prettify #21

Conversation

zertosh commented May 15, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zertosh May 15, 2017 • edited Loading

Choose a reason for hiding this comment

not-an-aardvark May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjeux left a comment

Choose a reason for hiding this comment

zertosh commented May 15, 2017

not-an-aardvark commented May 15, 2017

zertosh commented May 16, 2017

not-an-aardvark commented May 16, 2017

zertosh commented May 16, 2017

not-an-aardvark commented May 16, 2017

not-an-aardvark commented May 16, 2017

zertosh May 15, 2017 •

edited

Loading

not-an-aardvark May 15, 2017 •

edited

Loading