🤔 should we also have exists flag in the output? It probably makes sense to claim that a file is formattable only if it exists. Also the logic behind formattable gets a bit too complicated, don't you think?

should we also have exists flag in the output?

I don't have an opinion on this one way or the other.

Also the logic behind formattable gets a bit too complicated, don't you think?

Well, this logic needs to exist somewhere, right? It's better to do it once in the main repo than reproduced in all of the editor implementations. Or maybe I'm misunderstanding you?

I've looked over each of the scenarios you've documented, everything looks good to me!

Re formattable:

It's better to do it once in the main repo than reproduced in all of the editor implementations. Or maybe I'm misunderstanding you?

After a bit of thinking, I guess that moving this logic to the editor implementation is a better thing. The main reason is that there are two ways of formatting a file:

• from stdin / text argument + file name as option to determine parser
• from CLI for a real file with --write

In the first approach, it's OK to set formattable to true when a file does not exist – no harm at all. However, in the second scenario, just checking formattable for a path and then calling prettier --write could throw ENOENT (file does not exist). Now if we add exists flag and only trigger formattable if it's true, the first scenario breaks. So depending on what approach a tool like the Atom Package chooses, formattable will either work or not – it cannot be tuned so that it fits everyone.

What is also a bit confusing is that this flag may look over-promising. Someone may reasonably think that formattable: true may mean that prettier has already looked and the file syntax, successfully parsed it and has some changes to suggest. At least there is a chance that someone might interpret the name of the flag that way and come back with an issue.

I understand that implementing the logic of formattable within prettier-atom makes the if a lot bigger, but on the other hand, it removes some questions and does not lock everyone into a certain opinionated flavour of this flag. With formattable removed and exists: true/false added, the consumers of --file-info will be free to use whatever formatting approach they find best. Both stdin and prettier --write methods will be first-class choices.

New proposal:

{
  "exists": true,
  "ignored": false,
  "parser": "markdown", // can be null after 2.0
  "parserIsFallback": false // to be removed in 2.0
}

Or even (to simplify if today):

{
  "exists": true,
  "ignored": false,
  "inferredParser": "markdown", // can be null even in v1.x
  "parser": "markdown" // or babylon if inferredParser is null
   // parser can be removed in 2.0 as it will be the same as "inferredParser"
}

There is an internal function called inferParser, which already returns null when a fallback parser needs to be used.

Or actually we can even remove parser. 😅 It's inferredParser || "babylon" at all times anyway!

{
  "exists": true,
  "ignored": false,
  "inferredParser": "markdown", // or null, even in v1.x
}

WDYT? Will this make your if simple enough without formattable? Anything I forgot to take into account?

@robwise I can provide a prebuilt copy of prettier that you can test in prettier/prettier-atom#404. The reason why a prebuilt version is better is because it does not have eval() in plugin-related code, which Atom does not like (details).

Would you like me to change the result of getFileInfo() to the following before that?

{
  "exists": true,
  "ignored": false,
  "inferredParser": "markdown", // or null, even in v1.x
}

@kachkaev What happened to isFormattable? That's the really important part.

As far as eval, I'm already having to use atom/loophole for prettier-eslint so it's not the end of the world if we also start needing to use that for regular prettier.

Oh just saw the previous comment.

In the first approach, it's OK to set formattable to true when a file does not exist – no harm at all. However, in the second scenario, just checking formattable for a path and then calling prettier --write could throw ENOENT (file does not exist). Now if we add exists flag and only trigger formattable if it's true, the first scenario breaks. So depending on what approach a tool like the Atom Package chooses, formattable will either work or not – it cannot be tuned so that it fits everyone.

I'm not sure I understand this one, why would we return formattable: true if the file doesn't exist?

why would we return formattable: true if the file doesn't exist?

@robwise on first ‘format on save’. That is when the file name has already been given, there is some content to prettify, but the file has never been put on disk yet.

I'm still sceptical about adding formattable, details in #4341 (comment).

Apart from formattable existing or not, are you OK with these three options instead of the original ones? I'll update the implementation of so.

{
  "exists": true,
  "ignored": false,
  "inferredParser": "markdown", // or null, even in v1.x
}

The lack of parserIsFallback and the fact that inferredParser can be null, makes calculating formattable on your side less bulky:

if (fileInfo.exists && !fileInfo.ignored && fileInfo.inferredParser) {
  // format
}

or even like this to allow format on save for a file that has not been saved on disk yet:

if (!fileInfo.ignored && fileInfo.inferredParser) {
  // format 
}

IMO that's the way to go – prettier should not decide too much for the editor plugin.

@robwise on first ‘format on save’. That is when the file name has already been given, there is some content to prettify, but the file has never been put on disk yet.

I'm still confused about this point: if the file does not even exist on disk, why would we have Prettier claim it's formattable at that point? How would it know anything about the file at all—at this point, it doesn't exist except as a string of text inside of my editor's text buffer, and Prettier knows nothing about my editor's text buffer?

IMO that's the way to go – prettier should not decide too much for the editor plugin.

Well, agree to disagree, but I can definitely get the job done with what you've shown in your examples (where you demonstrate combining some of the flags), so 👍from me.

Apart from formattable existing or not, are you OK with these three options instead of the original ones? I'll update the implementation of so.

Yeah, the main differences being no formattable and no isFallbackParser? I'm fine with that IIUC.

@robwise done! I also rebased on top of the latest master and updated the first comment in this PR.

If you'd like to try out getFileInfo() in prettier/prettier-atom#404, feel free to npm install github:kachkaev/prettier#file-info-dist (with -dist in the end). It's a built version of my branch, which matches how prettier looks like on npm.

Waiting to hear your feedback and will proceed to writing tests after that.

if the file does not even exist on disk, why would we have Prettier claim it's formattable at that point?

In atom you would not be able to infer a parser while you don't know the file extension, i.e. until you save for the first time – that's true. But what about other editors? Imagine a hypothetical editor that always has a file path input field on top of a text area and you fill in both before you save for the first time. In this scenario, exists won't matter to derive formattable.

What I really like about having only exists, ignored and inferredParaser is that all these fields are orthogonal (i.e. they cannot be derived from each other). This means that there is no redundancy in the output.

If you're still worried about the size of an if statement in format on save, there's space for a new npm module that would shorten that for you 🙂

const { isFormattable } = require('prettier-file-info');

if (isFormattable(prettier.getFileInfo(filePath))) {
  // format on save
}

@suchipi could you please have a look at this PR? If you're happy with its design and implementation, I'll tick the first checkbox and proceed to writing tests. It'd be also great if we could go through #4192 before 1.13 because that PR is also important for text editors in a context of prettier plugins.

@kachkaev I'm getting an error:

path.js:28
    throw new TypeError('Path must be a string. Received ' + inspect(path));
    ^

TypeError: Path must be a string. Received undefined
    at assertPath (path.js:28:11)
    at Object.resolve (path.js:1188:7)
    at createIgnorer (/Users/rob/github/prettier-atom/node_modules/prettier/index.js:1743:39)
    at Object.getFileInfo (/Users/rob/github/prettier-atom/node_modules/prettier/index.js:24170:19)
    at Object.<anonymous> (/Users/rob/github/prettier-atom/test.js:8:27)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)

// test.js
const prettier = require('prettier')
const path = require('path')

const filePath = path.join(__dirname, 'test.js')
const fileInfo = prettier.getFileInfo(filePath)

console.log('file info:', JSON.stringify(fileInfo, null, 2))

@robwise – sorry for not mentioning what arguments prettier.getFileInfo() expects and so causing this exception on your side. The function has this API:

getFileInfo(
  filePath: string,
  opts: Options,
  ignorePath: string,
  doNotIgnoreNodeModules: boolean
): FileInfo

The reason why you were getting an exception was because you did not pass ignorePath, so an instance of ignorer could not be created.

This worked for me:

const filePath = path.join(__dirname, "test.js");
const ignorePath = path.join(__dirname, ".prettierignore");
const fileInfo = prettier.getFileInfo(filePath, {}, ignorePath);

or just

const fileInfo = prettier.getFileInfo("test.js", {}, '.prettierignore');

(paths are resolved based on cwd)

I'll make all arguments but the first one optional in the next few days, but you are free to test the method even before the changes. Apologies for not sharing this caveat with you – I was only testing the new functionality as the --file-info CLI option and did not think about it. My example two comments above is definitely wrong until I make all arguments but the first one optional.

No worries.

I was curious about the Options (which I'm presuming are the Prettier parsing options) as I assumed that they are irrelevant to just checking file info (no actual formatting is done here)?

I was thinking it might be more something like this, that way you aren't forced to provide an ignorePath in order to specify doNotIgnoreNodeModules:

type getFileInfo = (
  filePath: string, 
  opts?: { configPath?: string, ignorePath?: string, doNotIgnoreNodeModules?: boolean }
) => FileInfo;

Fair point, I’ll check that. I was not sure that mixing the options with two additional parameters is a good thing, but if the same is done in other methods, I’m happy to go for the trick.

Apart from this, do things work for you in general? I’m keen to cover the new feature with tests and remove the PR’s WIP status.

@kachkaev All seems to work so far, now we're able to get rid of that scopes thing entirely :)

@kachkaev index.js has been significantly changed in master. Can you get the latest from master and update the PR? Thanks!

@duailibe sure, just give me a few days ;–)

No hurry!

@duailibe the changes have been rebased on top of the latest master.

@robwise thanks to your suggestion and the new internal withPlugins() function, I was able to simplify the API of getFileInfo() It looks like this now:

const fileInfo = getFileInfo({
  filepath: 'path/to/file.js',
  ignorePath: 'path/to/.prettierignore', // optional, '.prettierignore' by default, can be set to false for disabling
  withNodeModules: true,  // optional, assumed false by default
  ...otherPrettierOptions, // optional
});

When getFileInfo() is called from CLI via --file-info, ignorePath is set to .prettierignore by default. I've decided to give this option the same default value in method call for consistency. If the file does not exist in cwd, nothing dramatic happens – an instance of ignorer is still created and this withNodeModules still works. As far as I understand, an exception is only thrown when a file at ignorePath cannot be read due to permission issues.

It's unfortunate that filepath is not camelCase while ignorePath is. This is because Prettier uses filepath everywhere else inside options and it's probably not a good idea to change this. I'm not naming ignorePath ignorepath for consistency with --ignore-path and other variable names throughout the code (filepath is the only exception here).

If everyone is happy with the new design, I'm happy to proceed to tests to remove WIP status from this PR.

@kachkaev sorry it took me so long to look at this, the API looks good to me. I'm not super familiar with the stuff in src/cli/util.js so I defer to you and the other maintainers on that part.

@suchipi great to hear that you're OK with the API (hope you looked at the version in my last commit as I forgot to update the first one and only did this now). Could you please cc someone who could look at src/cli/util.js?

Yeah, I looked at your most recent commit.

@ikatyang @azz @duailibe @lydell @j-f1 could one of you take a look at this?

@kachkaev if I don't pass an ignorePath, it's going to look for .prettierignore in the project's root directory, not in the directory of the filepath, right?

@robwise I guess it will be $(cwd)/.prettierignore – anything else would require too much changes throughout Prettier. Does it sound good to you?

While running Prettier in the context of a text editor, it’s probably worth setting this to project-dir/.prettierignore, because you know more about the env. Ideally, this file would be mentioned in prettier config, but Prettier does not support this yet.

@lipis in https://travis-ci.org/prettier/prettier/builds/376725574 – yes, while in https://travis-ci.org/prettier/prettier/builds/376719918 it went well (that's a commit with the same content). Do you see some button that would manually restart https://travis-ci.org/prettier/prettier/jobs/376725575? If not, I'll have to force-push again and again until all four jobs succeed.

There was button indeed, to restart only that job. Let's see :) 🕥

All green!

Squashed and rebased on top of the latest master that contains #4192 (had to squash because placing intermediate commits on top of master was creating conflicts). Old commits are still available in branch file-info-before-rebase-03 of my fork (for history).

I'll sortly add a test case for --plugin-search-dir as suggested previously.

Everything will be squashed anyways.. so no worries about the history! It's mostly for you

All done 🎉 I guess the PR is ready to be merged after someone checks out my last two commits.

the second to last is OK :P

@kachkaev Why was this made async?

@robwise There's a sync and an async method.

@duailibe ah excellent, that makes things much easier!