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

Detect version manager the same way in all scripts #390

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

zachallaun
Copy link
Collaborator

start_lexical.sh and port_wrapper.sh were using different methods to detect the version manager. start_lexical.sh was updated more recently, so I standardized on that.

I also refactored the detection function slightly to echo the result instead of setting a global variable, since it was only used in one place and can be easily captured. This should also make it easier to extract this function into a shared utility if we desire.

Finally, start_lexical.sh wasn't doing anything to start the program in the background in non-detected version manager case. I don't fully understand why this is necessary in the first place, but I added it for consistency.

`start_lexical.sh` and `port_wrapper.sh` were using different methods to
detect the version manager. `start_lexical.sh` was updated more
recently, so I standardized on that.

I also refactored the detection function slightly to echo the result
instead of setting a global variable, since it was only used in one
place and can be easily captured. This should also make it easier to
extract this function into a shared utility if we desire.

Finally, `start_lexical.sh` wasn't doing anything to start the program
in the background in non-detected version manager case. I don't fully
understand why this is necessary in the first place, but I added it for
consistency.
Copy link
Collaborator

@scohen scohen left a comment

Choose a reason for hiding this comment

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

Bash is strange

Copy link
Collaborator

@Blond11516 Blond11516 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing that!

@scohen scohen merged commit aa1de2d into main Oct 12, 2023
@scohen scohen deleted the za-shell-version-manager branch October 12, 2023 01:42
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