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(config): Move config handling to core.ps1 #3242

Merged
merged 19 commits into from
May 10, 2019
Merged

Conversation

r15ch13
Copy link
Member

@r15ch13 r15ch13 commented Mar 18, 2019

  • Migrate ~\.scoop to ~\.config\scoop\config.json
    • ~\.config is used by other tools too
    • Proper highlighting in text editor because of .json file extension
  • Remove hashtable and hashtable_val functions because ConvertFrom-Json is enough
    • These functions are obsolete because ConvertFrom-Json can handle all data formats
  • Add PowerShell 6 time conversion to tests
  • Removes config.ps1 imports from bin\*.ps1 and libexec\*.ps1

scoop alias requires changes to remove the hashtable specific stuff.

Copy link
Member

@niheaven niheaven left a comment

Choose a reason for hiding this comment

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

Tested with no .scoop in $HOME, work fine with changed codes.

lib/core.ps1 Outdated Show resolved Hide resolved
lib/core.ps1 Outdated Show resolved Hide resolved
lib/core.ps1 Outdated Show resolved Hide resolved
@niheaven
Copy link
Member

When moving config, there are two warn() which is defined below and give errors (warn not defined). Should replace with hardcode or put #message(L138-L189) at top of core.ps1.

@niheaven
Copy link
Member

L83-L87: If config value is false, $scoopConfig.$name will be false and fall to else section even if there is $name in config...
Test case:

scoop config debug true # okay
scoop config debug false # okay
scoop config debug false # error

It can be fixed by replacing L83-L87 with

$scoopConfig | Add-Member -Force -NotePropertyMembers @{ $name = $value }

That is, force add note member (we want to set it, so force is okay, no matter if there is $name)

@niheaven
Copy link
Member

niheaven commented Mar 18, 2019

Another implementation: pre-defined config file that contains all config options with default value, so we could only modify existing value, no need to add new ones. The pre-defined one can be put in scoop's root dir and copy to config home during first run. That may help users to know what configurable options is scoop having.

@r15ch13 r15ch13 force-pushed the refactor-config branch 3 times, most recently from 5c7cb51 to 69fea43 Compare March 25, 2019 15:34
lib/core.ps1 Outdated Show resolved Hide resolved
Copy link
Member

@chawyehsu chawyehsu left a comment

Choose a reason for hiding this comment

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

LGTM

@niheaven
Copy link
Member

There is only one thing remaining... Since set_config has output now, we should Out-Null it when we don't need, otherwise, e.g., scoop update will produce:

λ  scoop update
Updating Scoop...
Updating 'Ash258' bucket...
Updating 'dorado' bucket...
Updating 'extras' bucket...
Updating 'java' bucket...
Updating 'nerd-fonts' bucket...
Updating 'nih' bucket...
Updating 'rasa' bucket...
Updating 'versions' bucket...


SCOOP_BRANCH           : master
rootPath               : E:\Scoop
aria2-enabled          : False
msiextract_use_lessmsi : True
globalPath             : E:\ScoopG
debug                  : False
lastupdate             : 2019-03-27T11:20:17.0666758+08:00
cachePath              : E:\ScoopC
SCOOP_REPO             : https://github.com/lukesampson/scoop

Scoop was updated successfully!


Black lines above is added by scoop itself, that's messy, I think.

@r15ch13 r15ch13 changed the base branch from master to develop May 9, 2019 18:26
@r15ch13
Copy link
Member Author

r15ch13 commented May 9, 2019

Rebased onto develop and added missing | Out-Null redirections. Please test again 😄

Copy link
Member

@niheaven niheaven left a comment

Choose a reason for hiding this comment

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

LGTM

@r15ch13 r15ch13 merged commit 7699bea into develop May 10, 2019
@r15ch13 r15ch13 deleted the refactor-config branch May 11, 2019 16:44
r15ch13 added a commit that referenced this pull request May 12, 2019
- The main bucket of Scoop has been separated to https://github.com/ScoopInstaller/Main
- Requires PowerShell 5 and newer #3330
- Improve and refactor decompression functions #3204 #3409 #3411
- Move scoop config from `~\.scoop` to `~\.config\scoop\config.json` #3242
- Add `scoop hold` and `scoop unhold` for excluding programs from updates #3444
- Refactored some Core functions #3314 #3416
- Small fix for starting processes on Windows 7 #3415

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
Co-authored-by: Chawye Hsu <h404bi@outlook.com>
Co-authored-by: Jakub Čábera <cabera.jakub@gmail.com>
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.

None yet

3 participants