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

set activate workspace in .wmill/activeWorkspace in workspace directory #5085

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wenbinye
Copy link

@wenbinye wenbinye commented Jan 17, 2025

Set activate workspace in .wmill/activeWorkspace in workspace directory by command
wmill workspace switch <workspace>
The workspace activate when switch to the directory automatic.


Important

Prioritize setting and getting active workspace from .wmill/activeWorkspace in workspace.ts, with fallback to global store.

  • Behavior:
    • getActiveWorkspaceName() now prioritizes reading from .wmill/activeWorkspace before falling back to the global store.
    • setActiveWorkspace() writes to .wmill/activeWorkspace if .wmill directory exists, otherwise writes to global store.
  • Functions:
    • Modifies getActiveWorkspaceName() and setActiveWorkspace() in workspace.ts to handle local and global workspace activation.
  • Logging:
    • Logs a message if .wmill directory is not found when setting active workspace.

This description was created by Ellipsis for cbd81f9. It will automatically update as commits are pushed.

@wenbinye wenbinye requested a review from rubenfiszel as a code owner January 17, 2025 15:36
Copy link
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to cbd81f9 in 17 seconds

More details
  • Looked at 35 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. cli/workspace.ts:128
  • Draft comment:
    Consider using Deno.lstatSync instead of Deno.statSync to check if '.wmill' is a directory. lstatSync is more appropriate for checking the existence of a directory or file.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code attempts to check if '.wmill' is a directory using Deno.statSync, which can throw an error if the directory does not exist. This is correctly handled by the try-catch block, but the logic can be improved by using Deno.lstatSync, which is more appropriate for checking the existence of a directory or file.
2. cli/workspace.ts:43
  • Draft comment:
    Consider handling the case where the '.wmill/activeWorkspace' file might be empty, which could lead to unexpected behavior.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses Deno.readTextFile to read the active workspace file. However, it does not handle the case where the file might be empty, which could lead to unexpected behavior. Adding a check for an empty file would improve robustness.
3. cli/workspace.ts:46
  • Draft comment:
    Consider handling the case where the 'activeWorkspace' file might be empty, which could lead to unexpected behavior. This is also applicable in the previous Deno.readTextFile call.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses Deno.readTextFile to read the active workspace file. However, it does not handle the case where the file might be empty, which could lead to unexpected behavior. Adding a check for an empty file would improve robustness. This is applicable in both places where Deno.readTextFile is used for reading the active workspace.

Workflow ID: wflow_w9qg5BOXAnbagP4e


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel
Copy link
Contributor

Thanks for the contribution.

It's a nice idea and I did want to do it too. It's lacking a bit of discovery and relying on the existence or not of .wmill which I'm not a fan.

I suggest to have an option: --persist-in-folder to the switch command that will indeed write there (and create dir if necessary) but otherwise it's not written there. So it is used by default only if .wmill/activeInstance in which case a log mention that it is used and how to override it or delete it.

When it's not used because it doesn't exist, a log mention that it's using the global setting because no .wmill/activeInstance was found

@rubenfiszel rubenfiszel force-pushed the main branch 3 times, most recently from 2af98cd to 90ba65a Compare February 5, 2025 15:32
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.

2 participants