CLI: Support .ignore files #2412

alexander-akait · 2017-07-06T16:38:13Z

Add --ignore-path argument and support .prettierignore

lydell · 2017-07-06T16:40:36Z

bin/prettier.js

@@ -243,6 +249,10 @@ if (argv["help"] || (!filepatterns.length && !stdin)) {
      "                           This option cannot be used with --cursor-offset.\n" +
      "                           Defaults to Infinity.\n" +
      "  --no-color               Do not colorize error messages.\n" +
+      "  --ignore-path <path|string>  \n" +
+      "                           Path to a file containing patterns that describe files to ignore. The\n" +
+      "                           path can be absolute or relative to process.cwd(). By default, stylelint\n" +


stylelint → Prettier

@lydell sorry for stupid error 😄

vjeux · 2017-07-06T16:49:20Z

Thanks for the PR. I'm now convinced that we do need to address this problem inside of prettier.

I'd like to take a step back and figure out what the end state will look like, then once we agree with it, we can start with PRs.

I see three big things:

Options
Files to run it on
Files to ignore

Do we want three files for those? Do we want a single file? Should those files be JavaScript, JSON, plain text? Different for all those or all the same?

If you could tell me what you have in mind, that would be awesome!

vjeux · 2017-07-06T16:50:37Z

Also, what's the relationship between the CLI and the CommonJS version of the API. Should only the CLI part read from those files, or both?

alexander-akait · 2017-07-06T17:11:58Z

@vjeux

Options

Be good implement .prettierrc (and other formats, using cosmiconfig), but we can implement this late.

Files to run it on

I did not quite understand what your mean. If your mean which files we can handle, multiple patterns are enough for most use case.

Files to ignore

Do this as eslint and stylelint, support multiple patterns (already done) and --ignore-path.
Option --ignore-path only for file, like .eslintignore and .stylelintignore (.gitignore stylistics), no JavaScript or JSON, only plain text.

Also, what's the relationship between the CLI and the CommonJS version of the API. Should only the CLI part read from those files, or both?

Be good support this in the CommonJS version of the API, move some CLI logic to prettier (config, glob, ignore) inside run function (maybe best name).

But we can do this in other PR. We already support multiple pattern, here we can add --ignore-path and then support configuration using .prettierrc, then move this logic into prettier api.

CiGit · 2017-07-06T20:12:26Z

As an API consumer (prettier-vscode), I would be grateful if the CommonJS would be on par with the cli.
It would allow a more uniform integration between those editor plugins and the cli.

If it isn't resolved through the API for whatever reason, it would be great to have API calls like
prettier.getConfig(filePath): PrettierOption
prettier.isIgnored(filePath): boolean
I won't have to duplicate prettier's code :-) And certainly other API consumers.

alexander-akait · 2017-07-07T12:03:40Z

/cc @vjeux what do your think about this:

Merge this PR.
Implement configuration support .prettierignore for CLI.
Move logic from CLI to prettier.run?

Seems step by step allow implement this without big difficult PR and easy review?

vjeux · 2017-07-07T14:02:53Z

I'm sorry I'm currently in vacation with no access to a computer, I'll be able to do a proper review only next week.

Some other thoughts I have:

For the docs website, we transform index.js, if we move this fs logic there we need to make sure it still works.

I'm very concerned about performance if we put this inside of index.js, the naive version would be to do all the fs.stat/fs.read for every single call, but I fear that it's inefficient.

This means that we'd need to introduce a watcher for all those paths which comes with their own sets of issues.

I'm not sure what's the right solution is yet. I really want prettier to stay in the dozens of ms range for use in the editor. Otherwise it makes it a much worse experience for the developer.

alexander-akait · 2017-07-07T14:06:27Z

@vjeux So I think step by step implement will allow found good solution for this, without problems with perf 👍 But this pr only for cli 😄

vjeux · 2017-07-07T14:13:07Z

I'm more interested in figuring out what the end result will look like. Once we know what it is, i'll just merge all the pull requests quickly, the implementation is not really the problem.

Sorry for the delay!

azz · 2017-07-10T14:12:30Z

bin/prettier.js

@@ -243,6 +248,10 @@ if (argv["help"] || (!filepatterns.length && !stdin)) {
      "                           This option cannot be used with --cursor-offset.\n" +
      "                           Defaults to Infinity.\n" +
      "  --no-color               Do not colorize error messages.\n" +
+      "  --ignore-path <path|string>  \n" +
+      "                           Path to a file containing patterns that describe files to ignore. The\n" +


Can we simplify this a bit?

Path to a file containing patterns that describe files to ignore. Defaults to ./.prettierignore.

azz · 2017-07-10T14:14:17Z

bin/prettier.js

+  // The ignorer will be used to filter file paths after the glob is checked,
+  // before any files are actually read
+  const ignoreFilePath = options.ignorePath || DEFAULT_IGNORE_FILENAME;
+  const absoluteIgnoreFilePath = path.isAbsolute(ignoreFilePath)


I think this can just be:

const ignoreFilePath = path.resolve(options.ignorePath);

You don't need to explicitly pass process.cwd().

azz · 2017-07-10T14:14:51Z

bin/prettier.js

@@ -42,7 +44,8 @@ const argv = minimist(process.argv.slice(2), {
    "cursor-offset",
    "range-start",
    "range-end",
-    "stdin-filepath"
+    "stdin-filepath",
+    "ignore-path"


Add add a default value in the minimist options.

azz · 2017-07-10T14:15:33Z

bin/prettier.js

@@ -72,6 +75,8 @@ const ignoreNodeModulesGlobs = ["!**/node_modules/**", "!./node_modules/**"];
 const globOptions = {
  dot: true
 };
+const DEFAULT_IGNORE_FILENAME = ".prettierignore";
+const FILE_NOT_FOUND_ERROR_CODE = "ENOENT";


I don't think we need a constant for this, just inline the string.

azz · 2017-07-10T14:18:50Z

bin/prettier.js

@@ -382,6 +409,8 @@ function eachFilename(patterns, callback) {
        return;
      }

+      filePaths = ignorer.filter(filePaths);
+
      filePaths.forEach(filePath => {


ignorer.filter(filePaths).forEach(filePath => { ...

azz · 2017-07-10T14:20:56Z

bin/prettier.js

+    ignoreText = fs.readFileSync(absoluteIgnoreFilePath, "utf8");
+  } catch (readError) {
+    if (readError.code !== FILE_NOT_FOUND_ERROR_CODE) {
+      throw readError;


Probably do something like this rather than throwing:

console.error(`Unable to read ${ignoreFilePath}: ${readError.toString()}`); process.exit(2);

Could we log the full error so that we get the stack as well? It is often helpful for debugging.

console.error(`Unable to read ${ignoreFilePath}:`, readError); process.exit(2);

azz · 2017-07-10T14:24:37Z

bin/prettier.js

+    }
+  }
+
+  const ignorer = ignore().add(ignoreText.split(/\r?\n/));


That .split will create an array [ "" ] if ignoreText is "".

Might need to do this:

const ignorer = ignore(); if (ignoreText) { ignorer.add(ignoreText.split(/\r?\n/)); }

levithomason · 2017-07-11T01:54:03Z

bin/prettier.js

@@ -382,6 +409,8 @@ function eachFilename(patterns, callback) {
        return;
      }

+      filePaths = ignorer.filter(filePaths);
+
      filePaths.forEach(filePath => {
        return callback(filePath);


While you're at cleanup here, return does nothing in a forEach loop. This can just be:

ignorer.filter(filePaths).forEach(callback);

alexander-akait · 2017-07-11T11:44:07Z

/cc @azz @levithomason friendly ping

azz · 2017-07-11T12:32:27Z

Configuration file (.prettierrc) format was recently merged (#2434).

Prettier will look for configuration files starting at the file being formatted, and looking up the tree.

Should we be doing the same thing for .prettierignore? Maybe we can even re-use the configuration file for ignoring files, too:

// .prettierrc
{
  "ignore": [
    "*.min.js"
  ]
}

I know eslint has two files (.eslintrc and .eslintignore), but eslint config files can be much more complex than a .prettierrc.

Thoughts?

alexander-akait · 2017-07-11T12:45:23Z

@azz we should support .prettierignore (and allow customize this value). Why? In many project i have .gitignore where put all files which i want to ignore, i use --ignore-path .gitignore for eslint, stylelint and other linters. If we don't support customize --ignore-path, I will have to duplicate all ignored files in the .prettierrc and .gitignore. It will be extremely inconvenient (sometimes it is many many files).

azz · 2017-07-11T12:53:46Z

bin/prettier.js

  ],
  default: {
-    color: true
+    color: true,
+    "ignore-path": null


Sorry, might not have been clear before. This should be "ignore-path": ".prettierignore", then you don't need DEFAULT_IGNORE_PATH.

azz · 2017-07-11T12:55:33Z

Ok sounds good. Just one more small change and we can merge this.

azz · 2017-07-11T12:56:51Z

bin/prettier.js

@@ -283,6 +288,9 @@ if (
      "  --list-different or -l   Print filenames of files that are different from Prettier formatting.\n" +
      "  --config                 Path to a prettier configuration file (.prettierrc, package.json, prettier.config.js).\n" +
      "  --resolve-config <path>  Resolve the path to a configuration file for a given input file.\n" +
+      "  --ignore-path <path|string>  \n" +


Why <path|string>?

@azz your can use /path/to/.ignored (path) or .ignored (string), but we can simplify just use <path>

alexander-akait · 2017-07-11T13:20:33Z

@azz done

vjeux · 2017-07-11T13:24:58Z

@azz: @evilebottnawi doesn't have commit access (yet?) so you need to merge it yourself :)

azz · 2017-07-11T13:26:05Z

@CiGit To support this in prettier-vscode (for format-on-save), would you end up just reading the .prettierignore file yourself, or does it make sense to expose an API function for this (I doubt it)?

azz · 2017-07-11T13:35:24Z

Thanks @evilebottnawi 🎉

CiGit · 2017-07-11T13:43:36Z

@azz I may implement that logic, certainly copy-paste that one. Reading quickly through the code, it seems, unlike gitignore, to only read first encountered file, won't be that tough.

I still see some advantage to provide an API (code duplication)

Avoid copy-paste for all tooling based on prettier.
If the resolution algorithm changes, no need to copy-paste again.

Drawback :

An other exposed API.
Other ?

azz · 2017-07-11T13:54:36Z

It isn't 100% copy-paste, you want to use the .ignores() function instead of .filter().

ignore()
  .add(fs.readFileSync(".prettierignore", "utf8"))
  .ignores(currentFilePath);

azz · 2017-07-11T13:57:06Z

bin/prettier.js

+
+  const ignorer = ignore();
+
+  if (ignoreText.trim()) {


Turns out we don't need to split the string, you can just ignorer.add(ignoreText), @evilebottnawi do you want to open another quick PR?

@azz according docs we should pass array https://github.com/kaelzhang/node-ignore#usage

Seems to support both: https://github.com/kaelzhang/node-ignore#addignorefilepath

@azz i.e. just (without check on empty?):

const ignorer = ignore().add(ignoreText);

@azz done in #2455

CiGit · 2017-07-11T14:09:37Z

@azz good point.
I still don't get why most people emphasis on ignoring for formatOnSave. Why not always?

azz · 2017-07-11T14:11:42Z

Its bad UX to format something on save that should be formatted. When it is an explicit action (like alt-shift-f) the UX is ok.

CiGit · 2017-07-11T14:51:05Z

I agree with that. However, why would you ignore a file?

Don't use prettier on it. In which case preventing it's formatting is ok to me, preventing mistakes.
Don't waste time with the cli on files I don't care. IMO can be formatted on save in this case.

I don't see a use case for prettierignore which would prevent only onSave.
Disclaimer: I'm NOT an UX engineer.

lydell reviewed Jul 6, 2017

View reviewed changes

alexander-akait force-pushed the issue-2294 branch from cc2b802 to 567a03b Compare July 6, 2017 17:13

azz mentioned this pull request Jul 7, 2017

Support @prettier pragma comment #2397

Closed

azz changed the title ~~feat(cli): allow to specify ignored files using ignore file~~ CLI: Support .ignore files Jul 10, 2017

azz requested changes Jul 10, 2017

View reviewed changes

levithomason reviewed Jul 11, 2017

View reviewed changes

alexander-akait force-pushed the issue-2294 branch 3 times, most recently from 5f74f3b to 17ff3a5 Compare July 11, 2017 11:33

azz requested changes Jul 11, 2017

View reviewed changes

azz reviewed Jul 11, 2017

View reviewed changes

feat(cli): allow to specify ignored files using ignore file

602d4c3

alexander-akait force-pushed the issue-2294 branch from 17ff3a5 to 602d4c3 Compare July 11, 2017 13:09

azz approved these changes Jul 11, 2017

View reviewed changes

azz merged commit 469059d into prettier:master Jul 11, 2017

alexander-akait deleted the issue-2294 branch July 11, 2017 13:36

azz reviewed Jul 11, 2017

View reviewed changes

azu mentioned this pull request Jul 11, 2017

style(prettier): introduce prettier almin/almin#227

Merged

This was referenced Jul 11, 2017

Create option to ignore files (like webpack.config.js) prettier/prettier-vscode#57

Closed

[RFC] Enable/disable prettier formatting using status bar prettier/prettier-vscode#169

Closed

kimjoar mentioned this pull request Jul 25, 2017

Handle buggy ignore in Prettier elastic/kibana#12997

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: Support .ignore files #2412

CLI: Support .ignore files #2412

CLI: Support .ignore files #2412

CLI: Support .ignore files #2412

Conversation

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