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

Rewrite script #126

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

darkdecoy
Copy link

Updated script to take advantage of functions and for loops to clean up the code

backup.sh Outdated Show resolved Hide resolved
backup.sh Outdated Show resolved Hide resolved
@prodrigestivill
Copy link
Owner

Thanks for your dediction to this project, I have some comments.

This changes shouldn't be passing the CI in 78e8389, and yet it has successfully passed while containing typos. This happened, first because I didn't code a lot of CI test for all the cases, but also because the current tests depend on the script to setup the environment correctly, when moving the set tag into setup function this has been lost. Also adding this set in each function will make the script even longer.

On the other hand I undestand that "more redeable" can be an opinable topic. But I'm currently find those changes to lead to a more difficult to undestand code to my self. Mostly more dificult to read the diferent cases since they are now further apart in the code, as well as creation and cleanup of the same files are also too separated one from another to detect mistakes easily.

Also I'm awere that in the current form, the variable names aren't very well explanatory, and I value that you had done a better job to clarify the variable intencionality.

In any case, not sure what to ask/expect from this PR.

What for sure we have been detected are some issues already present with old pg versions we might need to remove.

@darkdecoy
Copy link
Author

The big change I would say that this makes is that takes the repetitive tasks where you are doing the same thing multiple times and puts them in for loops which makes them easier to manage.

I moved things out of functions so that they execute so that things are next to each other. I have some other pull requests that I would like to submit they are changes that the for loops make easier to implement.

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