Merge with eslint-plugin-prettify

[zertosh]

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.

[not-an-aardvark]

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

[not-an-aardvark]

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.)

[zertosh]

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?

[not-an-aardvark]

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.

[zertosh]

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]

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?

[zertosh]

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]

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.

[zertosh]

👍

[not-an-aardvark]

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?

[zertosh]

👍

[not-an-aardvark]

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.)

[zertosh]

👍

[not-an-aardvark]

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.

[zertosh]

👍

[not-an-aardvark]

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

[zertosh]

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]

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.

[zertosh]

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]

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.

[zertosh]

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]

Huh, TIL.

[not-an-aardvark]

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?

[zertosh]

👍

[vjeux]

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

[zertosh]

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]

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]

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]

Will do. Are you zertosh on npm?

[zertosh]

@not-an-aardvark, yup! Thanks!

[not-an-aardvark]

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

[not-an-aardvark]

Added you to the npm package.