-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
add __verbose() (IDFGH-3923) #5816
Conversation
ca351a9
to
47c8e29
Compare
47c8e29
to
4656b8f
Compare
Thanks for contribution. |
Thanks for the PR @rwalker-apple! I will combine this with the other proposed change to export.sh, which fixes the return code, if you don't mind: #5744. Edit: this will also need similar changes in export.bat, export.ps1 and export.fish. We can do them on top of your commit unless you prefer to handle that. |
@@ -11,8 +11,12 @@ realpath_int() { | |||
echo "$scriptdir" | |||
} | |||
|
|||
__verbose() { | |||
[[ -n ${IDF_QUIET} ]] && return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, i just ran shellcheck and realized that this [[
won't work in dash
. Can fix this up while rebasing on the other PR i've mentioned or leave this to you, if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing, how do you feel about using IDF_EXPORT_QUIET or IDF_TOOLS_QUIET instead of IDF_QUIET? I'm afraid that IDF_QUIET would imply to users that all the tools would disable output. Meeting that expectation would require quite a bit more changes to be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@igrr please take it from here and do as you see fit. Just some suggestions. Thanks for the portability check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@igrr are you going to assume responsibility for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, have missed your previous comment. Sure, can take it from here. Thank you.
@igrr, do you intend to work on this branch, or should I close the PR? |
Let's keep it open, and it should get closed automatically when the combined changes from the PR and #5744 are merged into master. |
Rob Walker seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Problem
export.sh is noisy for projects that invoke the script for every build
Proposed changes