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

Use set +x / -x to hide the whole ynh_script_progression computation #741

Merged

Conversation

Projects
None yet
2 participants
@alexAubin
Copy link
Member

commented Jun 19, 2019

The problem

ynh_script_progression is awesome, but it fills the output log with a huge bunch of boring lines related to the computation of the progress bar :[

Solution

Add some set +x / -x to hide this computation

PR Status

Tested and working on my machine

How to test

Run an app install like wordpress and compare the full output

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@@ -280,6 +282,7 @@ ynh_script_progression () {
fi

ynh_print_info "[$progression_bar] > ${message}${print_exec_time}"
set -x

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis Jun 19, 2019

Contributor

Globally agree with it, but don't you think this set -x should be just before the ynh_print_info, so we would have the echo into the log. Useful to know where the script is.
Didn't try though...

This comment has been minimized.

Copy link
@alexAubin

alexAubin Jun 19, 2019

Author Member

Uh but the echo will be in the log as set +x/-x only affects the debug output of Bash

In fact, there will automatically be a set -x inside ynh_print_info because it relies on getopts, which itself also uses set -x

So from what I remember, the only usefulness of putting this set -x here (rather than before) is to hide a few additional local foo at the beginning of ynh_print_info

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis Jun 19, 2019

Contributor

Huh, is there any use of that last set -x as you said indeed, getops will do it.

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

So uh proposing to merge soonish ? Not sure what to do about the set -x ... (it's mainly there to be explicit instead of having to guess that the other helper will do it)

@alexAubin alexAubin added this to the 3.6.x milestone Jun 26, 2019

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

It won't harm anyway

@alexAubin alexAubin merged commit aa25fff into stretch-unstable Jun 29, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the less-madness-in-debug-log-from-script-progression branch Jun 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.