Account for multiple project roots #1609

PEZ · 2022-03-20T17:52:36Z

Glob for all project roots in the workspace

Let the user pick if there are more than one
Changes the way we find the closest project root
Not using node fs module at all
Fixes #1254

My Calva PR Checklist

I have:

Ping @PEZ, @bpringe

Let the user pick if there are more than one Changes the way we find the closest project root Not using node fs module at all Fixes #1254

corasaurus-hex

I had a couple questions but other than that this is beautiful 😚👌🏻

corasaurus-hex · 2022-03-20T19:20:32Z

src/state.ts

+async function findProjectRootPaths() {
+  const projectFileNames: string[] = ['project.clj', 'shadow-cljs.edn', 'deps.edn'];
+  const projectFilesGlob = `**/{${projectFileNames.join(',')}}`;
+  const excludeDirsGlob = `**/{${config.getConfig().projectRootsSearchExclude.join(',')}}`;


I see target is in the list of excluded directories. Would that exclude a directory named target-files or does it need to be just target by itself in order to match this glob?

It will only exclude target, exactly as that. Wherever it appears, though...

corasaurus-hex · 2022-03-20T19:22:58Z

src/state.ts

+  console.debug('glob took', new Date().getTime() - t0, 'ms');
+  const projectFilePaths = candidateUris.map((uri) => path.dirname(uri.fsPath));
+  const candidatePaths = [...new Set(projectFilePaths)];
+  console.log(candidatePaths);


It might be cool to make this console.log({candidatePaths}) so the log entry contents are labeled?

TIL.

Wasn't intending to keep this logging in there, though. Except the timing of the glob, which could be good to be able to ask users about for a while.

corasaurus-hex · 2022-03-20T19:24:19Z

src/state.ts

+    ? candidatePaths
+        .filter((p) => docDir.startsWith(p))
+        .sort()
+        .reverse()[0]


Nice! That's a good way to find the longest match 😄

corasaurus-hex · 2022-03-20T19:30:54Z

src/config.ts

@@ -161,6 +161,7 @@ function getConfig() {
      useDeprecatedAliasFlag: configOptions.get<boolean>('jackIn.useDeprecatedAliasFlag'),
    },
    enableClojureLspOnStart: configOptions.get<boolean>('enableClojureLspOnStart'),
+    projectRootsSearchExclude: configOptions.get<string[]>('projectRootsSearchExclude'),


Oh! I missed one thing! We should probably add a default here so that it satisfies the gods of strictNullChecks:

configOptions.get<string[]>('projectRootsSearchExclude') ?? []

Thanks. I used the default value signature of the get:

configOptions.get<string[]>('projectRootsSearchExclude', [])

Does that also satisfy the rule? Less syntax to parse...

ooooh, I didn't know about that one! I think that should fix it? and If not I can fix it later when i'm fixing null check things 😊

And generallt just clean up the logic some

PEZ · 2022-03-21T18:06:04Z

@Cyrik : can you have a look at e356cfd and see if you think it is viable way to deal with the quick-pick menus in e2e tests? I didn't figure out a very nice way to select the correct sequence...

I'd be awesome if @corasaurus-hex and @bpringe had a look as well.

❤️

PEZ · 2022-03-21T20:28:56Z

I wonder why that saveAs doesn't do the trick now... I'll have a closer look. Thanks!

corasaurus-hex

I think this looks great! I love to see code improvements like this.

corasaurus-hex · 2022-03-21T21:32:09Z

src/connector.ts

+    let projectDirUri = state.getProjectRootUri();
+    if (!projectDirUri) {
+      projectDirUri = await state.getOrCreateNonProjectRoot(context, true);
+    }


it looks like getOrCreateNonProjectRoot calls getProjectRootUri internally, first thing, and returns it later. Could we ditch the getProjectRootUri here and just call getOrCreateNonProjectRoot on its own here? Or should we remove the getProjectRootUri from getOrCreateNonProjectRoot? Or just leave everything as-is?

I'll have to check and think. You are probably right. This has been somewhat cleaned up, but I might have missed this opportunity. Tomorrow, though, because right now too 💤 !

corasaurus-hex · 2022-03-21T21:35:12Z

src/connector.ts

+    if (!projectDirUri) {
+      projectDirUri = await state.getOrCreateNonProjectRoot(context, true);
+    }
+    await state.initProjectDir(projectDirUri);


We add a .catch to this function in connectCommand, do we need one here?

Iirc i added the one in connectCommand because the user can do funny stuff with the menu. Here we don't allow that. But it could also go wrong because of non-validated glob patterns, so yes, we should have one here anyway. =)

corasaurus-hex · 2022-03-21T21:41:18Z

src/utilities.ts

@@ -547,6 +558,7 @@ export {
  logSuccess,
  getCljsReplStartCode,
  getShadowCljsReplStartCode,
+  quickPickActive as quicPickActive,


Can we get rid of the as quicPickActive here? We'll need to fix the references to it in the tests, too.

No idea why this coercion is here... I'll ditch it!

PEZ · 2022-03-21T22:14:15Z

I love to see code improvements like this.

❤️

When I tried to extract the find-root-paths stuff to a module named project-root.ts, it turned out there was already such a module! And it had yet another implementation of the same traversing! Crazy. I just replaced it with the new stuff and updated the only call site. It seems to work. But also makes us glob more often, of course. Maybe we should keep the other way to find closest root around anyway...

docs/site/connect.md

src/project-root.ts

Co-authored-by: Brandon Ringe <[email protected]>

…morrow/calva into 1254-multiple-project-roots

PEZ · 2022-03-22T16:48:56Z

🎉

PEZ added 2 commits March 20, 2022 18:49

Glob for all project roots in the workspace

4ed9d80

Let the user pick if there are more than one Changes the way we find the closest project root Not using node fs module at all Fixes #1254

prettier

8e8f3ee

PEZ requested review from bpringe, corasaurus-hex and Cyrik March 20, 2022 17:52

corasaurus-hex approved these changes Mar 20, 2022

View reviewed changes

corasaurus-hex reviewed Mar 20, 2022

View reviewed changes

PEZ added 2 commits March 21, 2022 09:17

Update docs about monorepo support

de75dde

Update connect to avoid two root menus

e742b32

And generallt just clean up the logic some

PEZ marked this pull request as ready for review March 21, 2022 08:20

PEZ added 5 commits March 21, 2022 09:23

prettier

20c13d0

Add title option to quickPickSingle

3dfaae4

Sort the project roots alphabetically

43b5bab

Provide testDirUri to initProjectDir to make e2e test avoid menu

bd69189

Experimental quickPick selection for e2e tests

e356cfd

corasaurus-hex approved these changes Mar 21, 2022

View reviewed changes

PEZ added 3 commits March 21, 2022 22:49

Move project root stuff to project-root module

c114b61

Less stateful way to get project root

fa251e7

prettier

40ab919

bpringe requested changes Mar 22, 2022

View reviewed changes

docs/site/connect.md Outdated Show resolved Hide resolved

src/project-root.ts Outdated Show resolved Hide resolved

PEZ and others added 6 commits March 22, 2022 07:11

Update src/project-root.ts

7871dbc

Co-authored-by: Brandon Ringe <[email protected]>

Fix typo

a505492

Typo

d3827b2

Clean up some more statefulness

d2d923e

Merge branch '1254-multiple-project-roots' of github.com:BetterThanTo…

749aa31

…morrow/calva into 1254-multiple-project-roots

Remove some debug logging

4d5527e

PEZ merged commit 00abd8b into dev Mar 22, 2022

PEZ deleted the 1254-multiple-project-roots branch March 22, 2022 16:48

PEZ mentioned this pull request Mar 23, 2022

Connect fails when there is no project file (deps.edn, etc) #1613

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for multiple project roots #1609

Account for multiple project roots #1609

PEZ commented Mar 20, 2022 •

edited

Loading

corasaurus-hex left a comment

corasaurus-hex Mar 20, 2022

PEZ Mar 20, 2022

corasaurus-hex Mar 20, 2022

PEZ Mar 20, 2022

corasaurus-hex Mar 20, 2022

corasaurus-hex Mar 20, 2022

PEZ Mar 21, 2022

corasaurus-hex Mar 21, 2022

PEZ commented Mar 21, 2022

PEZ commented Mar 21, 2022

corasaurus-hex left a comment

corasaurus-hex Mar 21, 2022

PEZ Mar 21, 2022

corasaurus-hex Mar 21, 2022

PEZ Mar 21, 2022

corasaurus-hex Mar 21, 2022

PEZ Mar 21, 2022

PEZ commented Mar 21, 2022

PEZ commented Mar 22, 2022

Account for multiple project roots #1609

Account for multiple project roots #1609

Conversation

PEZ commented Mar 20, 2022 • edited Loading

My Calva PR Checklist

corasaurus-hex left a 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

PEZ commented Mar 21, 2022

PEZ commented Mar 21, 2022

corasaurus-hex left a 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

PEZ commented Mar 21, 2022

PEZ commented Mar 22, 2022

PEZ commented Mar 20, 2022 •

edited

Loading