Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch CLI to async #10804

Merged
merged 10 commits into from May 9, 2021
Merged

Switch CLI to async #10804

merged 10 commits into from May 9, 2021

Conversation

fisker
Copy link
Member

@fisker fisker commented Apr 30, 2021

Description

This PR didn't actually use any async API in CLI, just make it async and make test pass.
This is a necessary step for #4459 and #4980

Finally, I figured how to test it.

There is still one failed test, I can't debug it on windows, maybe @thorn0 or @connor4312 can help me out? Fixed

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@fisker fisker closed this Apr 30, 2021
@fisker

This comment has been minimized.

@fisker fisker reopened this Apr 30, 2021
@fisker fisker changed the title Switch CLI to async (take 2) Switch CLI to async Apr 30, 2021
@connor4312
Copy link
Contributor

connor4312 commented Apr 30, 2021

Awesome to see this 🎉 This will make parallel running possible. I was hesitant to get cracking on that before due to probable massive merge conflicts if I moved it to async at the same time.


For the test, just moving the creation of directories into beforeAll fixes it for me. Guessing these may have been clobbered by concurrently-running tests or something.

patch
diff --git a/tests/integration/__tests__/patterns-dirs.js b/tests/integration/__tests__/patterns-dirs.js
index 39f7f986f..27d129566 100644
--- a/tests/integration/__tests__/patterns-dirs.js
+++ b/tests/integration/__tests__/patterns-dirs.js
@@ -1,7 +1,7 @@
 "use strict";
 
 const path = require("path");
-// const fs = require("fs");
+const fs = require("fs");
 const runPrettier = require("../runPrettier");
 const { projectRoot } = require("../env");
 
@@ -87,38 +87,37 @@ describe("plugins `*`", () => {
   });
 });
 
-// TODO: fix this test
-// if (path.sep === "/") {
-//   // Don't use snapshots in these tests as they're conditionally executed on non-Windows only.
-
-//   const base = path.resolve(__dirname, "../cli/patterns-dirs");
-
-//   // We can't commit these dirs without causing problems on Windows.
-
-//   // TODO: these should be moved to a `beforeAll`, but for that to be possible,
-//   // `runPrettier` should be refactored to use `describe` and `beforeEach` for doing setup.
-//   fs.mkdirSync(path.resolve(base, "test-a\\"));
-//   fs.writeFileSync(path.resolve(base, "test-a\\", "test.js"), "x");
-//   fs.mkdirSync(path.resolve(base, "test-b\\?"));
-//   fs.writeFileSync(path.resolve(base, "test-b\\?", "test.js"), "x");
-
-//   describe("Backslashes in names", () => {
-//     afterAll(() => {
-//       fs.unlinkSync(path.resolve(base, "test-a\\", "test.js"));
-//       fs.rmdirSync(path.resolve(base, "test-a\\"));
-//       fs.unlinkSync(path.resolve(base, "test-b\\?", "test.js"));
-//       fs.rmdirSync(path.resolve(base, "test-b\\?"));
-//     });
-
-//     testPatterns("", ["test-a\\/test.js"], { stdout: "test-a\\/test.js\n" });
-//     testPatterns("", ["test-a\\"], { stdout: "test-a\\/test.js\n" });
-//     testPatterns("", ["test-a*/*"], { stdout: "test-a\\/test.js\n" });
-
-//     testPatterns("", ["test-b\\?/test.js"], { stdout: "test-b\\?/test.js\n" });
-//     testPatterns("", ["test-b\\?"], { stdout: "test-b\\?/test.js\n" });
-//     testPatterns("", ["test-b*/*"], { stdout: "test-b\\?/test.js\n" });
-//   });
-// }
+if (path.sep === "/") {
+  // Don't use snapshots in these tests as they're conditionally executed on non-Windows only.
+
+  const base = path.resolve(__dirname, "../cli/patterns-dirs");
+
+  // We can't commit these dirs without causing problems on Windows.
+
+  describe("Backslashes in names", () => {
+    beforeAll(() => {
+      fs.mkdirSync(path.resolve(base, "test-a\\"));
+      fs.writeFileSync(path.resolve(base, "test-a\\", "test.js"), "x");
+      fs.mkdirSync(path.resolve(base, "test-b\\?"));
+      fs.writeFileSync(path.resolve(base, "test-b\\?", "test.js"), "x");
+    });
+
+    afterAll(() => {
+      fs.unlinkSync(path.resolve(base, "test-a\\", "test.js"));
+      fs.rmdirSync(path.resolve(base, "test-a\\"));
+      fs.unlinkSync(path.resolve(base, "test-b\\?", "test.js"));
+      fs.rmdirSync(path.resolve(base, "test-b\\?"));
+    });
+
+    testPatterns("", ["test-a\\/test.js"], { stdout: "test-a\\/test.js\n" });
+    testPatterns("", ["test-a\\"], { stdout: "test-a\\/test.js\n" });
+    testPatterns("", ["test-a*/*"], { stdout: "test-a\\/test.js\n" });
+
+    testPatterns("", ["test-b\\?/test.js"], { stdout: "test-b\\?/test.js\n" });
+    testPatterns("", ["test-b\\?"], { stdout: "test-b\\?/test.js\n" });
+    testPatterns("", ["test-b*/*"], { stdout: "test-b\\?/test.js\n" });
+  });
+}
 
 function testPatterns(namePrefix, cliArgs, expected = {}) {
   const testName =

@fisker
Copy link
Member Author

fisker commented Apr 30, 2021

@connor4312 Thank you! I'll update when I get home.

fisker and others added 2 commits May 1, 2021
Co-authored-by: Connor Peet <connor@peet.io>
@connor4312
Copy link
Contributor

connor4312 commented May 1, 2021

🚀

Are you planning on starting to move the CLI internals async as well in the near future? I will need that for parallel running--asyncifying the code paths to the parts where I spawn workers at least. I can tackle some of that if not.

@fisker
Copy link
Member Author

fisker commented May 1, 2021

Maybe, but no promise, a little busy recently.

@connor4312
Copy link
Contributor

connor4312 commented May 1, 2021

Ok. If I get some time this weekend, I'll start on that. Maybe it would be easiest to collaborate on a single branch to avoid too much conflict. Do you want to make a fork/branch of prettier that I can write to? Or I can do so.

@fisker fisker requested a review from a team May 1, 2021
tests/integration/runPrettier.js Outdated Show resolved Hide resolved
thorn0
thorn0 approved these changes May 8, 2021
Co-authored-by: Georgii Dolzhykov <thorn.mailbox@gmail.com>
@thorn0 thorn0 merged commit 164a6e2 into prettier:main May 9, 2021
26 checks passed
@fisker fisker deleted the async-cli-2 branch May 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants