test: resolve configs when --stdin-filepath is used
#58
Conversation
Copies the stdin-filepath tests from prettier. Notable differences: - Blocked by prettier#21 - Syntax errors output the `stdin-filepath` basename in prettier but not in this CLI (e.g. `[Error] foo.js SyntaxError blah`)
src/config_editorconfig.ts
Outdated
| const fileName = filesNames[i]; | ||
| const filePath = fastJoinedPath(folderPath, fileName); | ||
| if (!Known.hasFilePath(filePath)) continue; | ||
| if (!ignoreKnown && !Known.hasFilePath(filePath)) continue; |
There was a problem hiding this comment.
Not a huge fan as ignoreKnown needs to be passed through multiple hoops before it ends up here, but I'm not sure how else we could solve this (getStdin doesn't populate Known, and so we'd always end up not reading any config).
I'm also not sure whether this impacts performance, i.e. the whole chain – not populating Known in runStdin and resolving the configs without it.
There was a problem hiding this comment.
In Prettier v3, this file lives in a bigger overarching fixture directory (config), but it's needed for the snapshots to match what we expect. We could consider removing it, but then we need to update the snapshots to include semicolons.
| @@ -0,0 +1,8 @@ | |||
| endOfLine: 'auto' | |||
| overrides: | |||
| - files: "**/*.js" | |||
There was a problem hiding this comment.
In Prettier v3, this is just "*.js", but a/a/a/a/a/a/three.js matches that there somehow (maybe they do some exploding to include the leading **?). Since we usually provide nested paths in --stdin-path, the overrides wouldn't apply without **/.
There was a problem hiding this comment.
prettier treats *.ext as **/*.ext. not through any exploding/splitting, it just prepends it iirc
There was a problem hiding this comment.
Is this something that we should do in v4 too or rather require the users to handle it explicitly?
There was a problem hiding this comment.
there was a conversation a while back around it. im not sure if it was in discord or on here
but iirc, fisker's opinion was that it makes patterns easier to write (*.ts to format all ts files is easier than **/*.ts). even though it isn't the right glob
we should probably discuss it again and make a decision
| // TODO: This is currently a false positive as no config actually gets resolved, but Prettier | ||
| // somehow formats the input correctly anyway. | ||
| describe("apply editorconfig for stdin-filepath in root", () => { | ||
| const code = dedent` | ||
| function f() { | ||
| console.log("should be indented with a tab"); | ||
| } | ||
| `; | ||
| runCli("", ["--stdin-filepath", "/foo.js"], { | ||
| input: code, // js | ||
| }).test({ | ||
| status: 0, | ||
| stdout: code, | ||
| stderr: "", | ||
| write: [], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I'm not sure what we expect here. The test case's name implies that we should search for any .editorconfig (either starting from root or from CWD) and apply it (especially since this test is only ran in CI in v3), but it seems like an odd choice.
| // TODO: This is currently a false positive. Gotta investigate how it's handled in Prettier v3 to | ||
| // gauge the expected behavior. | ||
| describe("don't apply editorconfig outside project for stdin-filepath with nonexistent directory", () => { | ||
| runCli( | ||
| "", | ||
| [ | ||
| "--stdin-filepath", | ||
| "editorconfig/repo-root/nonexistent/one/two/three.js", | ||
| ], | ||
| { | ||
| input: dedent` | ||
| function f() { | ||
| console.log("should be indented with 2 spaces"); | ||
| } | ||
| `, // js | ||
| }, | ||
| ).test({ | ||
| status: 0, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
It's currently a false positive, but when we change the first argument in runCli to editorconfig and update the path accordingly, stdin does get formatted based on __fixtures__/editorconfig/.editorconfig. Not sure why it wouldn't based on the other test cases. Maybe it's because repo-root counts as a separate project due to .hg there? Such an edge case could be quite tricky to support.
| const ignoreNames = options.ignore ? [".gitignore", ".prettierignore"] : []; | ||
| const isIgnored = await getIgnoreResolved(fileName, ignoreNames, true); | ||
| if (isIgnored) { | ||
| stdout.always(trimFinalNewline(fileContent)); |
There was a problem hiding this comment.
This matches the v3 behavior based on the related test case, where the ignored file gets output as is.
src/index.ts
Outdated
| const ignoreNames = options.ignore ? [".gitignore", ".prettierignore"] : []; | ||
| const isIgnored = await getIgnoreResolved(fileName, ignoreNames, true); |
There was a problem hiding this comment.
The logic is a bit more involved in runGlobs, but we don't have a way to explicitly include a file when handling the stdin case, so it's quite simplified.
…stdin-filepath-test-fixes
|
|
||
| async function run(options: Options, pluginsDefaultOptions: PluginsOptions, pluginsCustomOptions: PluginsOptions): Promise<void> { | ||
| if (options.globs.length || !isString(await getStdin())) { | ||
| if (options.globs.length || (!isString(await getStdin()) && !("stdinFilepath" in options))) { |
There was a problem hiding this comment.
can you explain this one?
if there is stdin and there is a stdinFilepath, we now runGlobs?
and if there's no stdin but there is a stdinFilepath, we'd also runGlobs?
unless im reading wrong, thats what its doing now
There was a problem hiding this comment.
I based this branch on yours, you seem to have added this condition in 585f1be. Not really sure what the rationale was, but it seems to make sense, right? We runGlobs either when options.globs is not empty or there's no stdinFilepath in options AND there's no stdin data.
There was a problem hiding this comment.
if there's no stdinFilePath but there is stdin, we'll now call runStdin i think?
so i probably got this condition wrong. i don't remember why i did it that way
if there's no stdin or there's globs, we should runGlobs. otherwise, we can runStdin
2 participants
Based on #52.
TODOs: