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

refactor(core): Rewrite and separate path-related functions to system.ps1 #5836

Merged
merged 7 commits into from
Mar 27, 2024

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Mar 20, 2024

Description

Ed. Separate part of isolate PATH to #5840, this PR is for refactoring now.

Use config option use_isolated_path to separate Scoop apps PATH to SCOOP_PATH, and just add %SCOOP_PATH% to $env:Path when install app with env_add_path first time.

Motivation and Context

What the commits do:

  • refactor(core): Remove unused ensure_robocopy_in_path()
  • refactor(system): Move EnvVar-related functions to system.ps1
    • Move from core.ps1: Publish-Env(), env(), search_in_path(), ensure_in_path(), strip_path(), add_first_in_path(), remove_from_path()
    • Add necessary system.ps1 importing
  • refactor(EnvVar): Separate env() to Get-EnvVar() and Set-EnvVar()
  • refactor(system): Remove search_in_path()
    • Replace with Get-Command().Source
  • refactor(system): Refactor PATH-related functions
    • strip_path() -> Find-Path()
    • ensure_in_path()/add_first_in_path() -> Add-Path()
      • ensure_in_path() is only used in shim creation
      • Use -Force to forcedly add a path in the first position as add_first_in_path()
      • Add-Path() supports %
    • remove_from_path() -> Remove-Path()

How Has This Been Tested?

Passed all Scoop tests.

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

@niheaven niheaven force-pushed the refactor-system branch 2 times, most recently from d7f0580 to 6a3fbed Compare March 21, 2024 09:33
- `strip_path()` -> `Find-Path()`
- `ensure_in_path()`/`add_first_in_path()` -> `Add-Path()`
- `remove_from_path()` -> `Remove-Path()`
@niheaven niheaven force-pushed the refactor-system branch 2 times, most recently from 7241009 to e88260e Compare March 22, 2024 02:23
@niheaven niheaven changed the title feat(path): Isolate Scoop apps' PATH refactor(core): Rewrite and separate path-related functions to system.ps1 Mar 22, 2024
lib/system.ps1 Outdated Show resolved Hide resolved
lib/system.ps1 Outdated
}

function strip_path($orig_path, $dir) {
Show-DeprecatedWarning $MyInvocation 'Find-Path'
Copy link
Member

@chawyehsu chawyehsu Mar 27, 2024

Choose a reason for hiding this comment

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

I realize there are references of this function in some manifests, but I think this function should not be provided and used outside of the core (though I know there is impossible to hide a function based on the current corebase, all functions are exposed, which I dislike).

What they really want is a way to manipulate $env:PATH-like environment variables, something like Add-PathLikeEnvVar and Remove-PathLikeEnvVar (further abstraction of Add-Path and Remove-Path). This is useful because there are many operations on $env:PATH-like env vars, plus the proposal of $env:SCOOP_PATH is staging. Add-Path and Remove-Path are $env:PATH-specific and may not be much useful if/after $env:SCOOP_PATH is implemented.

Copy link
Member Author

@niheaven niheaven Mar 27, 2024

Choose a reason for hiding this comment

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

Maybe a refactoring into modules could be done if I have time.

For now, keep the deprecated warning of strip_path, and will remove outside use after this PR goes to main.

@niheaven niheaven merged commit 6772e61 into develop Mar 27, 2024
2 checks passed
@niheaven niheaven deleted the refactor-system branch March 27, 2024 09: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