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.

Sign up for GitHub

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

Jump to bottom

Add projectRootDir for monorepo connection sequences #1100

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Add projectRootDir for monorepo connection sequences #1100

wants to merge 2 commits into from

Conversation

stefan-toubia
Copy link
Contributor

What has Changed?

Adds a projectRootDir option for custom connection sequences to specify a root dir to use instead of resolving the root dir based on the current open file context.

Fixes #579

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
  • Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. NB: There is a CircleCI bug that makes the Artifacts hard to find. Please see this issue for workarounds.
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • Referenced the issue I am fixing/addressing in a commit message for the pull request.
    • If I am fixing the issue, I have used GitHub's fixes/closes syntax
    • If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
  • Created the issue I am fixing/addressing, if it was not present.
  • Added to or updated docs in this branch, if appropriate

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

  • Made sure the PR is directed at the dev branch (unless reasons).
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and tested it there if so.
  • Read the source changes.
  • Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse, @bpringe

@stefan-toubia stefan-toubia requested a review from PEZ April 3, 2021 18:41
@stefan-toubia stefan-toubia mentioned this pull request Apr 3, 2021
Closed
17 tasks
@stefan-toubia stefan-toubia changed the title 579 project root dir Add projectRootDir for monorepo connection sequences Apr 3, 2021
@stefan-toubia
Copy link
Contributor Author

There's still one issue to resolve with this, currently when you jack-in the results doc is created before we prompt for the connection sequence and potentially change the project root dir based on this new setting. This is causing the results doc to become disconnected due to the directory change. There's a couple potential solutions:

  1. Reorder the sequence within jackIn so that initResultsDoc is only called after we get the connection sequence and potentially reinitialize the project dir. This would require changing the 'aborting jack-in' message since this currently writes out to the window. This could be an alert popup instead.
  2. Instead of picking the results doc output location based on project root dir, make the location static. This could be based on the workspace folder, a user-defined path in settings, or just cache the first file path returned by outputFileDir. This would also solve the issue of there being multiple output file windows open at a time, which seems like an improvement to me but I'm not sure if anybody finds it beneficial to have more than one results doc open at a time.

My preference would be 2 since it also solves the multiple results doc issue, WDYT?

@PEZ
Copy link
Collaborator

PEZ commented Apr 3, 2021

I think option 2 would be strange when you are working with more than one project. And option 1 sounds like it would risk opening a can of worms.

Do we have other options?

@bpringe
Copy link
Member

bpringe commented Apr 5, 2021

I think option 2 would be strange when you are working with more than one project

@PEZ Can you elaborate more on this? I don't work with multiple projects simultaneously much, so maybe it's hard for me to see the issue. Do you mean contextually? Like, I'm working on A and B, and I run some things in A and then switch to B and run some things, and now there's output from two different projects in the output window, but it's unclear from which project they came?

If that is the case, I don't see that being an issue, since output is output and happened in the past. I may be way off here from what you're thinking though. Maybe you also mean the context for working directly in the output/repl window could be confusing ("which project am I in?") - though there is the namespace in the prompt, at least.

@PEZ
Copy link
Collaborator

PEZ commented Apr 6, 2021

@bpringe you basically elaborated it to what I was thinking about. The difference in viewpoint being that I see a huge issue with it. 😄 I have five to ten different projects open almost all the time and having some system.log kind of output would be awful.

I think that can of worms (option 1) is interesting. We've pulled off similar things in the past. Maybe it is not as scary as I first thought it was.

@PEZ PEZ closed this Apr 6, 2021
@PEZ PEZ deleted the 579-project-root-dir branch April 6, 2021 06:46
@bpringe
Copy link
Member

bpringe commented Apr 6, 2021

I have five to ten different projects open almost all the time and having some system.log kind of output would be awful.

Yeah, I see. Good point.

@stefan-toubia
Copy link
Contributor Author

@PEZ did you intend to delete this branch?

@bpringe
Copy link
Member

bpringe commented Apr 6, 2021

I was wondering the same. I know he did some cleanup earlier. 🤔

@PEZ
Copy link
Collaborator

PEZ commented Apr 6, 2021

My apologies! Not intended. At. All.

@bpringe bpringe restored the 579-project-root-dir branch April 7, 2021 00:15
@bpringe bpringe reopened this Apr 7, 2021
PEZ
PEZ reviewed Apr 7, 2021
View reviewed changes
package.json
@@ -3,7 +3,7 @@
"displayName": "Calva: Clojure & ClojureScript Interactive Programming",
"description": "Integrated REPL, formatter, Paredit, and more. Powered by cider-nrepl and clojure-lsp.",
"icon": "assets/calva.png",
"version": "2.0.185",
"version": "2.0.185-monorepo-proj-root-dir",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"version": "2.0.185-monorepo-proj-root-dir",
"version": "2.0.185",

(I usually pick up the VSIX from CircleCI instead. Maybe we should make a script that build a local VSIX...)

PEZ
PEZ reviewed Apr 7, 2021
View reviewed changes
package.json
@@ -373,6 +373,11 @@
"description": "Here you can give Calva some Clojure code to evaluate in the CLJ REPL, once it has been created.",
"required": false
},
"projectRootDir": {
"type": "string",
"descripion": "The root directory to execute the Jack-in command from. This will override the default behavior for determining the root directory relative to the current open file. This is useful for working in monorepos.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI (and this should go to the dev docs): I've started to use markdownDescription to boost the schema help a bit.

PEZ
PEZ reviewed Apr 7, 2021
View reviewed changes
src/nrepl/connectSequence.ts
Comment on lines +212 to +215
const sequences = customSequences.filter(customSequence =>
!!customSequence.projectRootDir
|| defSequenceProjectTypes.includes(customSequence.projectType)
).concat(defSequences);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite remember where the result from this function is used. Can you explain why you filter away sequences with the setting? (I'm sure it is the right thing to do, just want to understand why. 😄 )

@PEZ
Copy link
Collaborator

PEZ commented Apr 7, 2021

Good morning. Again my apologies for deleting the branch. Thanks for restoring, @bpringe!

I left some comments in the review. Overall it looks good. I wonder if you have tested this with the Connect to a running REPL scenario? I expected that there would need to be some changes in the connect.ts module as well.

@mrkam2
Copy link
Contributor

mrkam2 commented Apr 30, 2021

@stefan-toubia bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PEZ PEZ PEZ left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@stefan-toubia @PEZ @bpringe @mrkam2