Skip to content

refactor: Use common script to avoid code duplication#60

Merged
ChrisTitusTech merged 2 commits intoChrisTitusTech:mainfrom
lj3954:use_common_script
Jul 23, 2024
Merged

refactor: Use common script to avoid code duplication#60
ChrisTitusTech merged 2 commits intoChrisTitusTech:mainfrom
lj3954:use_common_script

Conversation

@lj3954
Copy link
Copy Markdown
Contributor

@lj3954 lj3954 commented Jul 21, 2024

This proposed change includes creating a macro to concatenate the common script with the specified script. A macro must be used, since macros (including the concat macro) are evaluated before functions.

The shell will be launched through the shebang within the common script, shebangs within other scripts will be disregarded as comments when they reach the interpreter. This wouldn't cause issues with these scripts, but it should be noted in case the arguments to the shell need to be modified in the future.

Closes #28

@afonsofrancof
Copy link
Copy Markdown
Collaborator

afonsofrancof commented Jul 21, 2024

You might not have realized that different scripts have slightly different checkEnv functions.

I submitted a PR #32 a few days ago that correctly handled this behavior and even separated checkEnv into smaller functions.
It was rejected since I was sourcing the common script inside the shell scripts, instead of concatenating, like you are doing.
I think the best way to solve this is for you merge your changes to the rust code with mine to the shell scripts.

You can pull my PR to inspect it by cloning this repo (It's the one that stores the PRs) and pulling the PR

git clone https://github.com/ChrisTitusTech/linutil

git fetch origin pull/32/head:pr_32

git switch pr_32

alternatively if you have this repo as upstream in your fork's clone , just do

git fetch upstream pull/32/head:pr_32

git switch pr_32

If you want I can also merge my original changes to the scripts to your fork, which would then automatically be merged here if this gets accepted.

@afonsofrancof
Copy link
Copy Markdown
Collaborator

afonsofrancof commented Jul 21, 2024

Nice! I think you might need to update your branch to be compatible with the new main now.
They have changed system-update.sh, and it now supports "xbps-install", so it requires it to be added when checking the package manager, as well as to the fastUpdate function.

I realised now that there have been other changes to that file that need to be added aswell.

@lj3954 lj3954 force-pushed the use_common_script branch from ad19ee7 to d8600e8 Compare July 21, 2024 23:55
@lj3954
Copy link
Copy Markdown
Contributor Author

lj3954 commented Jul 22, 2024

That should be all that's needed. No changes are necessary to the fastUpdate function.

@afonsofrancof
Copy link
Copy Markdown
Collaborator

That should be all that's needed. No changes are necessary to the fastUpdate function.

Yup, my bad.
Just tested your PR and everything is working perfectly.

@ChrisTitusTech ChrisTitusTech merged commit df563ad into ChrisTitusTech:main Jul 23, 2024
@lj3954 lj3954 deleted the use_common_script branch July 23, 2024 20:49
@lj3954 lj3954 mentioned this pull request Sep 15, 2024
7 tasks
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.

Source common-script.sh in the other script instead of duplicating

3 participants