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

Issue #11637: Resolve shellcheck SC2016 #12841

Merged
merged 1 commit into from Mar 15, 2023

Conversation

ThatSneakyCoder
Copy link
Contributor

@ThatSneakyCoder
Copy link
Contributor Author

ThatSneakyCoder commented Mar 14, 2023

@nrmancuso @romani Solving this issue bought new warnings in picture (SC2154). Please check the cli output below:

  1. Output before making the necessary changes:
╭─shubh220922@fedora ~/Desktop/checkstyle_project/checkstyle ‹6b77f061b› 
╰─$ shellcheck .ci/*.sh   

In .ci/releasenotes-gen.sh line 55:
              -Dexec.args='${project.version}' \
                          ^------------------^ SC2016 (info): Expressions don't expand in single quotes, use double quotes for that.


In .ci/validation.sh line 12:
    '${base.dir}/../../target/[artifact]-[revision]-all.[ext]' \
    ^-- SC2016 (info): Expressions don't expand in single quotes, use double quotes for that.


In .ci/validation.sh line 773:
                     -Dexec.args='${checkstyle.version}' \
                                 ^---------------------^ SC2016 (info): Expressions don't expand in single quotes, use double quotes for that.

For more information:
  https://www.shellcheck.net/wiki/SC2016 -- Expressions don't expand in singl...

  1. Output after making the changes:
╭─shubh220922@fedora ~/Desktop/checkstyle_project/checkstyle ‹sc2016› 
╰─$ shellcheck .ci/*.sh   

In .ci/releasenotes-gen.sh line 55:
              -Dexec.args="${project.version}" \
                           ^----------------^ SC2154 (warning): project is referenced but not assigned.


In .ci/validation.sh line 12:
    "${base.dir}/../../target/[artifact]-[revision]-all.[ext]" \
     ^---------^ SC2154 (warning): base is referenced but not assigned.


In .ci/validation.sh line 773:
                     -Dexec.args="${checkstyle.version}" \
                                  ^-------------------^ SC2154 (warning): checkstyle is referenced but not assigned.

For more information:
  https://www.shellcheck.net/wiki/SC2154 -- base is referenced but not assign...

New type of warning has shown up. We will have to initialize the highlighted properties for this warning to go away as per https://www.shellcheck.net/wiki/SC2016

Edit 1:

@nrmancuso @romani

Can we make use of the uppercase scenario here as in the documentation (https://www.shellcheck.net/wiki/SC2154):

image

Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

@shubh220922 these look like system/maven properties that we do not want to expand, we can suppress each individually with comment explaining why.

edit: please make comment like we do not want to expand properties in this command

@nrmancuso nrmancuso self-assigned this Mar 14, 2023
@ThatSneakyCoder
Copy link
Contributor Author

@nrmancuso Done. Suppressions have been made at respective places. But, what is wrong with making a permanent suppression inshellcheckrc file?

@nrmancuso
Copy link
Member

@nrmancuso Done. Suppressions have been made at respective places. But, what is wrong with making a permanent suppression inshellcheckrc file?

While these all look like places that we do not want bash to expand a variable, there is no guarantee about future usages :)

Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

I am good as CI passes

@nrmancuso nrmancuso requested a review from romani March 14, 2023 20:17
@nrmancuso nrmancuso assigned rnveach and romani and unassigned nrmancuso and rnveach Mar 14, 2023
@ThatSneakyCoder
Copy link
Contributor Author

ThatSneakyCoder commented Mar 14, 2023

@nrmancuso
Copy link
Member

@shubh220922 failure in azure is not related to your changes. The failing job is mvn clean verify ran on MacOS.

@ThatSneakyCoder
Copy link
Contributor Author

@nrmancuso alright. But wouldn't merging this PR create mvn clean verify issue on MacOS as per the CI.

@nrmancuso
Copy link
Member

@shubh220922 it is probably just a random failure, we will have someone with access restart this job before we merge :)

@romani romani merged commit 9065407 into checkstyle:master Mar 15, 2023
@ThatSneakyCoder ThatSneakyCoder deleted the sc2016 branch March 20, 2023 18:44
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.

None yet

4 participants