Skip to content

Conversation

@hoffie
Copy link
Member

@hoffie hoffie commented Mar 23, 2022

Short description of changes

This PR applies the suggestions by shellcheck to all our .sh scripts.

Some suggestions do not apply. For those, a comment is added to disable the check and explain why shellcheck cannot properly judge it.

CHANGELOG: Internal: Made all shell scripts shellcheck-clean.

Context: Fixes an issue?

Related: #2474

Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

Ready for review.

What is missing until this pull request can be merged?

Final CI runs & reviews.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want (most of the non build-related scripts have been checked manually)
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

The previous logic used eval to dynamically create variables.
This is hard to follow.
Instead, explicitly name the relevant two variables.
Also, introduce readable variable names.

Related: jamulussoftware#2474
@hoffie hoffie added this to the Release 3.9.0 milestone Mar 23, 2022
@hoffie hoffie requested a review from ann0see March 23, 2022 23:33
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

I think the most important thing to check here is the mac_deploy.sh script. I'll check it on my VM and then approve.

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Tested on macOS and it works.

Some suggestions do not apply. For those, a comment is added to disable
the check and explain why shellcheck cannot properly judge it.

Related: jamulussoftware#2474
@hoffie hoffie force-pushed the autobuild/shellcheck-clean branch from e1abe9e to 06bffcc Compare March 28, 2022 07:39
@hoffie hoffie requested a review from pljones March 28, 2022 07:40
@ann0see
Copy link
Member

ann0see commented Apr 1, 2022

@pljones @hoffie can we merge this soon?

@hoffie hoffie merged commit 06cd9a8 into jamulussoftware:master Apr 5, 2022
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