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: de-global env #65

Merged
merged 18 commits into from
Dec 19, 2023
Merged

refactor: de-global env #65

merged 18 commits into from
Dec 19, 2023

Conversation

ComradeVanti
Copy link
Collaborator

@ComradeVanti ComradeVanti commented Dec 14, 2023

Goal

This is a preparation refactor for the next PR where I want to tackle CLI validation. After starting with that I noticed, that I would like to do validation on env stuff before we even get inside the cmd-function. To make implementation of that easier env was extracted all the way up into these cmd-functions so it can easily be moved one more step up into the cli-logic.

Changes

Besides some other small refactors, this pull requests cleans up the env logic in three major ways:

Remove unused properties

Some properties, such as color are only ever used inside the parseEnv function and can be removed from the env object.

Also env currently stored all registry auth information even though you will only ever need the one for the registries that are currently in use. This was also cleaned up.

Pull up env usages

Instead of referencing env deep inside utility functions only relevant pieces of information are passed in. env is now only used inside cmd-functions e.g. at the top level.

Un-global env

Since env is now created and only used inside of cmd-functions, the parseEnv function now returns the created env object instead of making it available globally.

Replace object parameters with separate parameters. This is mostly done for consistency, as most functions in the project use multiple parameters. Also it makes the function signature a lot shorter.
Remove env usage from pkg-manifest-io
Remove env usage from upm-config-io
Remove env from fetchPackageInfo
Remove usage from registry-client
Removes the _ functions that exist in some cmd files by either making them local functions inside the main cmd function or by just moving their content into the main cmd function.

The goals for this are:
- Move env usage up into just the env functions
- Avoid unnecessary parameter passing
Remove env from getNpmFetchOptions function
Remove env usage from manifest-assertions
Remove env usage from writeUnityToken
Manifest-path is no longer stored on env, but calculated on demand from cwd. The goal here is to shrink env / deduplicate information in it.

parseEnv still checks whether manifest-path exists
Make env a non-global value
instead of storing urls for regular and upstream registry, as well as all auths, now we store only the auths for the used registries and put them together with the registry urls
nmpAuth is only used inside the parse-env function and can be removed from env
color is only used inside the parse-env function and can be made local
region is only used inside the parse-env function and can be made local
@favoyang favoyang self-requested a review December 15, 2023 15:00
@favoyang favoyang self-assigned this Dec 15, 2023
This typo caused color to be used in test scenarios which would mess with the mock console
If the color option is undefined it should also count as true
Copy link
Member

@favoyang favoyang left a comment

Choose a reason for hiding this comment

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

LGTM

@favoyang favoyang merged commit 2c0bdab into openupm:master Dec 19, 2023
2 checks passed
@ComradeVanti ComradeVanti deleted the env branch December 20, 2023 14: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