Update dev_setup.sh to use and benefit from bash double square bracket syntax #2166
Conversation
…t syntax + Consequently use double square brackets (over single square backets), to be consistent and benefit from this syntax + Skip obsolete quoting of left sided variables and strings in double square brackets + Use "-z" instead of comparing with empty string explicitly in double square brackets + Merge multiple double square bracket expressions, connected by "&&" or "||", into a single expression + "! -z" => "-n" + Remove obsolete free spaces from command substitutions, to be consistent
|
Hello, @MichaIng, thank you for helping with the Mycroft project! We welcome everyone To protect yourself, the project, and users of Mycroft technologies we require Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank |
There was a problem hiding this comment.
Thanks for providing this cleanup. I will test it further when the CLA arrives, from what I can see most look good. The only thing that caught my attention was the removal of quotes around some paths, there has been issues before with paths containing spaces.
But you probably know more of bash than me so if you can defend the change I'll accept it :)
|
@forslund |
devs-mycroft
added
CLA: Yes
There was a problem hiding this comment.
Looks good as far as I can tell. I had two comments that you can look at and say if they're things that needs to be handled. If you don't have time to look at it I'll just merge this PR tomorrow, it's a pretty nice improvement over the previous code.
Many thanks for contributing this!
dev_setup.sh
Outdated
| if [ ! -z ${INSTALL_PRECOMMIT_HOOK+x} ] || grep -q "MYCROFT DEV SETUP" ${HOOK_FILE}; then | ||
| if [ ! -f ${HOOK_FILE} ] || grep -q "MYCROFT DEV SETUP" ${HOOK_FILE} ; then | ||
| if [[ -n ${INSTALL_PRECOMMIT_HOOK} ]] || grep -q "MYCROFT DEV SETUP" ${HOOK_FILE}; then | ||
| if [[ ! -f ${HOOK_FILE} ]] || grep -q "MYCROFT DEV SETUP" ${HOOK_FILE} ; then |
There was a problem hiding this comment.
Nothing you have to fix but I wonder if it's an old bug here in the grep statement if there are spaces in the path?
There was a problem hiding this comment.
@forslund
The +x replaces the contained string with an x IF it exists, so it does not have spaces anymore. If the check is just about empty string or not, then the contained +x is actually an alternative to double quoting, since it works around possible space issues. But the curly braces can be removed safely if no manipulation (like +x) is done and no literal meant characters are added to the string.
Will add a commit to make optimal bash code 🙂.
There was a problem hiding this comment.
I might have been unclear, I was reffering to the grep command part of it: grep -q "MYCROFT DEV SETUP" ${HOOK_FILE} will ${HOOK_FILE} be expanded to a single argument if it the HOOK_FILE path has spaces is my question.
There was a problem hiding this comment.
@forslund
Ah yep, theoretically doubles quotes make sense here. However the variable is just assigned the lines above, clearly without spaces.
+ Always use single quotes around strings that are fully meant literal and do not contain single quotes themselves. + Remove curly braces around variable names, if no next character or a space is following. + Remove curly braces double quotes around single variables (and command substitutions) that are assigned to variables. The contained strings are always added correctly with spaces. + Merge multiple printed lines into one echo call.
|
Did some further tuning, I hope not too much for now. Basically:
|
|
I'm ok with these changes. Merging. |
Description
How to test
Run (especially the interactive part of) the installer with multiple command options and YN selections.
However, this is just a trivial minor syntax enhancement.
Contributor license agreement signed?