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

Add projectRootDir setting #580

Conversation

stefan-toubia
Copy link
Contributor

Hello again. This PR proposes a small but meaningful feature that will allow us users working in monorepos to configure our replConnectSequence with a specific project root directory. Previously, I've always had to open a specific file before doing jack-in. This addition removes that step, and a lot of headaches that came with it.

What has Changed?

  • Add projectRootDir setting to calva.connectionSequences.menuSelections to manually override the project root directory. This is useful for monorepo projects.

Addressed #579

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Made sure I am directing this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I am changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.) 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. (For now you'll need to opt in to the CircleCI New Experience UI to see the Artifacts tab, because bug.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • Created the issue I am fixing/addressing, if it was not present.
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.

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).
  • 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

@PEZ
Copy link
Collaborator

PEZ commented Mar 7, 2020

Thanks! I'll have a look and test this. Can you also update the user docs for custom sequences? It is often easiest to judge if this kind of change is applied on the right place, by reading the user docs. It all looks good at my quick reading of the diffs, and I might have some more feedback in a moment.

@stefan-toubia
Copy link
Contributor Author

Updated. My main area of uncertainty was exactly where to put this new property within the replConnectionSequences. I'm not sure I understand the difference between what's inside menuSelections vs what's on the same level, but it seems to fit inside menuSelections.

@PEZ
Copy link
Collaborator

PEZ commented Mar 8, 2020

menuSelections are meant for pre-filling/answering prompts that Calva would otherwise show the user. I think this fits better at the root level of the sequence, side by side with name and those.

The whole idea is a clever help for monorepos. I have been thinking quite a lot about how to support them better, but didn't think about how well it fits with a custom sequence. 😄

@stefan-toubia
Copy link
Contributor Author

Sounds good to me, I've moved it to the root level.

I've been using this new feature for the past couple days and it's definitely been a small but nice quality of life improvement! I can't speak for other monorepo users, but it works well for our code base. We recently gone through the process of switching most of our clojure projects from lein to Clojure CLI, and implemented a custom all/deps.edn file that lists all dependencies in our code base. Now doing jack-in on that directory lets us jump around all our projects without needing to restart. We did, however, have to fork the Clojure CLI tool to make a small fix to support our monorepo workflow.

@stefan-toubia stefan-toubia force-pushed the monorepo-proj-root-dir branch from 3f0e89a to 2218810 Compare March 9, 2020 02:39
CHANGELOG.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/nrepl/jack-in.ts Outdated Show resolved Hide resolved
src/state.ts Show resolved Hide resolved
@stefan-toubia stefan-toubia requested a review from PEZ March 11, 2020 02:14
Copy link
Collaborator

@PEZ PEZ left a comment

Choose a reason for hiding this comment

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

This is correct. 😄

@PEZ
Copy link
Collaborator

PEZ commented Mar 11, 2020

So, now I have merged things to dev that also deals with the project dir thing. Can you pull that in and see that your changes still work? Sorry about the hassle!

@stefan-toubia
Copy link
Contributor Author

Sorry for the delay, things have been a bit hectic with all this virus business. I will check this soon.

@PEZ
Copy link
Collaborator

PEZ commented Mar 17, 2020

No problem at all!

Here's a doc page that might benefit from a mention of/section about monorepos: https://calva.readthedocs.io/en/latest/workspace-layouts.html

@mrkam2
Copy link
Contributor

mrkam2 commented Feb 3, 2021

@stefan-toubia bump :-)

@PEZ
Copy link
Collaborator

PEZ commented Feb 3, 2021

I think things have moved quite a bit around this in Calva. Not sure which route is best, but maybe it is easiest to start over from a fresh dev and apply the relevant changes manually.

@stefan-toubia
Copy link
Contributor Author

Closing this, replaced by #1100

@stefan-toubia stefan-toubia deleted the monorepo-proj-root-dir branch April 3, 2021 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants