Skip to content

Conversation

@Bin-QA
Copy link
Contributor

@Bin-QA Bin-QA commented May 15, 2020

Update for fix shell check error for case-lib/*
expect case-lib/config.sh file

@Bin-QA Bin-QA requested review from marc-hb and xiulipan as code owners May 15, 2020 09:29
@Bin-QA
Copy link
Contributor Author

Bin-QA commented May 15, 2020

Wait for CI system detect error

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing all these fixes, will make future reviews much easier! See comments below.

case-lib/opt.sh Outdated
else
[ "${OPT_VALUE_lst[$i]}" -eq 0 ] && echo -e "\t""Default Value: Off" \
|| echo -e "\t""Default Value: On"
echo -e "\t""Default Value: On"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid echo, use printf

printf '\tDefault Value: '
if [ "${OPT_PARM_lst[$i]}" -eq 1 ]; then
   printf '%s\n' "${OPT_VALUE_lst[$i]}"
...
   printf 'On\n'

if [ $# -lt 1 ]; then
dlogi "Topology file name is not specified, unable to run command: ${BASH_SOURCE[-1]}" && exit 1
dlogi "Topology file name is not specified, unable to run command: $SCRIPT_NAME"
exit 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

die()

tplg_path=$(func_lib_get_tplg_path "$1") || {
dloge "No available topology for pipeline export"
exit 1
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

die()

@marc-hb
Copy link
Collaborator

marc-hb commented May 15, 2020

About the [WIP] : if you submit the PR as a "github draft" then no reviewer will be automatically added.
https://github.blog/2019-02-14-introducing-draft-pull-requests/

It's too late now, you can't go back to Draft. Next time.

@paulstelian97 paulstelian97 marked this pull request as draft May 18, 2020 08:07
@paulstelian97
Copy link

It's too late now, you can't go back to Draft. Next time.

Not too late apparently. It can always be converted back and review requests re-sent.

@paulstelian97 paulstelian97 removed the request for review from xiulipan May 18, 2020 08:08
Bin-QA added 6 commits May 19, 2020 10:57
fix shell check error and cleanup some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
1. remove _func_logcmd_add_timestamp function
direct to use 'alais' for the dlog* command
2. fix shell check error
3. remove some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
1. refine option for sof-tplgreader format
2. fix shell check error and remove some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
fix shell check error and remove some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
fix shell check error and remove some useless code

Signed-off-by: Wu, BinX <binx.wu@intel.com>
use readarray instead of IFS
readarray make code more simple and clean
It also fix shell check error

This change refer
hijack.sh: exit
lib.sh: func_lib_disable_pulseaudio
pipeline.sh: func_pipeline_export
@Bin-QA
Copy link
Contributor Author

Bin-QA commented May 19, 2020

@marc-hb

  1. die() function to fix I want to add into other PR,
    it also refer the problem we have different exit code
    so here die() just focus on dloge + exit 1?
  2. echo replace with print just for the opt.sh or other file

Already verify on CI system

@Bin-QA Bin-QA marked this pull request as ready for review May 19, 2020 05:44
@marc-hb
Copy link
Collaborator

marc-hb commented May 19, 2020

  1. I keep losing track of where die() is available versus not. If it's not available here yet then forget it for now.

  2. For very simple strings with no control characters echo is IMHO not the best but acceptable. For anything more advanced like for instance control characters please use printf.

@Bin-QA
Copy link
Contributor Author

Bin-QA commented May 20, 2020

  1. For very simple strings with no control characters echo is IMHO not the best but acceptable. For anything more advanced like for instance control characters please use printf.

@marc-hb I already try with printf
but I find, if I want use this style string "description need two\nlines" will not output
like my design, so it need to parse the string, how to deal those string for output

@paulstelian97
Copy link

but I find, if I want use this style string "description need two\nlines" will not output
like my design, so it need to parse the string, how to deal those string for output

Use single quotes on that, otherwise the shell will eat the \. Or do double escaping.

@marc-hb
Copy link
Collaborator

marc-hb commented May 20, 2020

Exactly: always single quote the format string:

printf 'format: \t%s\nstring: \t%d' "$arg1" "$arg2"
a='pri\nt_this'
echo -e "value:\t$a"
printf 'val:\t%s\n' "$a"

echo command with '-e' option will hide the control char
for the output string replace with printf '%s' to instead of it

Signed-off-by: Wu, BinX <binx.wu@intel.com>
@Bin-QA Bin-QA changed the title [WIP]fix shell check error for case-lib/* fix shell check error for case-lib/* May 21, 2020
@Bin-QA
Copy link
Contributor Author

Bin-QA commented May 25, 2020

@marc-hb Can you help me to review this patch, or you think what I need to update

@marc-hb marc-hb self-requested a review May 28, 2020 04:23
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marc-hb Can you help me to review this patch, or you think what I need to update

Sorry for the delay, this is a large PR and I don't really understand the changes unrelated to shellcheck. Next time please submit several smaller PRs, one at a time thanks!

@Bin-QA Bin-QA merged commit b4fe945 into thesofproject:master May 28, 2020
@Bin-QA Bin-QA deleted the shellcheck branch May 28, 2020 04:54
@marc-hb
Copy link
Collaborator

marc-hb commented Jul 1, 2020

Next time please submit several smaller PRs, one at a time thanks!

It's not only impossible to review changes that large, it's also impossible to test them in a reasonable time. See for instance regression #266

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.

3 participants