Skip to content

lib: opt.sh: remove useless cmd.txt code and shellcheck cleanup - #206

Closed
Bin-QA wants to merge 1 commit into
thesofproject:masterfrom
Bin-QA:cmd
Closed

lib: opt.sh: remove useless cmd.txt code and shellcheck cleanup#206
Bin-QA wants to merge 1 commit into
thesofproject:masterfrom
Bin-QA:cmd

Conversation

@Bin-QA

@Bin-QA Bin-QA commented Apr 23, 2020

Copy link
Copy Markdown
Contributor
  1. remove useless code
  2. fix shell check error for this file

Signed-off-by: Wu, BinX binx.wu@intel.com

@Bin-QA

Bin-QA commented Apr 23, 2020

Copy link
Copy Markdown
Contributor Author

Verify for the CI system, without script break

@marc-hb marc-hb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, removing code is always worth more than adding new code! Less maintenance. Can you explain in the commit message and summarize in the commit Subject: one-line why this code was not useful? Anymore? Removing code is the bigger change and should be first in the commit message. Shellchecks fixes are great too but they are secondary and should be second.

Comment thread case-lib/opt.sh
Comment thread case-lib/opt.sh Outdated
aiChaoSONG
aiChaoSONG previously approved these changes Apr 26, 2020

@aiChaoSONG aiChaoSONG left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@marc-hb marc-hb changed the title lib: opt.sh: cleanup for shellcheck lib: opt.sh: remove useless cmd.txt code and shellcheck cleanup Apr 26, 2020
Comment thread case-lib/opt.sh Outdated
Comment thread case-lib/opt.sh Outdated
Comment thread case-lib/opt.sh Outdated
Comment thread case-lib/lib.sh
marc-hb
marc-hb previously requested changes May 6, 2020

@marc-hb marc-hb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please run checkpatch locally first before submitting, it will save a lot of code review time and delays.

Comment thread case-lib/opt.sh
Comment thread case-lib/lib.sh
Comment thread case-lib/opt.sh
Comment thread case-lib/opt.sh Outdated
@Bin-QA
Bin-QA force-pushed the cmd branch 3 times, most recently from f7c668c to 9e88d29 Compare May 13, 2020 14:16
@marc-hb
marc-hb dismissed their stale review May 13, 2020 17:25

New version pushed

1. remove useless code for record cmdline into cmd.txt file
This file design for record full command line
but it is none to use it and it also have some issue
when full paste command
it too complex to fix full command line issue
so remove it
2. fix shell check error for opt.sh
3. move git_path as SCRIPT_HOME into lib.sh

Signed-off-by: Wu, BinX <binx.wu@intel.com>
@Bin-QA

Bin-QA commented May 15, 2020

Copy link
Copy Markdown
Contributor Author

too long time for this PR
and the target is miss match with original
I will create the new PR for the code clean and shell check fix

@Bin-QA Bin-QA closed this May 15, 2020
@marc-hb

marc-hb commented May 15, 2020

Copy link
Copy Markdown
Collaborator

I will create the new PR for the code clean and shell check fix

PR #222 correct?

@Bin-QA

Bin-QA commented May 18, 2020

Copy link
Copy Markdown
Contributor Author

I will create the new PR for the code clean and shell check fix

PR #222 correct?

Yes

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