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

Ignore shellcheck warning SC3043 globally in script #598

Closed

Conversation

rreinhardt9
Copy link

@rreinhardt9 rreinhardt9 commented Aug 30, 2021

Hello Thoughtbot!! Thanks for sharing this script with us all, it's served as an inspiration for me when working to develop my own laptop setup scripts ❤️

One thing I ran into when setting up this project locally is that shellcheck was warning about the use of local throughout the script. In #481 it looks like this was discussed and it was decided this was safe to ignore.

I found out that you can ignore a shellcheck warning for the entire file by adding it to the top. Doing so in this case might be helpful in order to:

  • Document the fact that we are ignoring the local warning directly in the file
  • Avoid developer confusion when they are setting the script up locally and get this linter warning right away.

Thought I'd share what I learned back in this PR in case it would be helpful. 🚀 No worries if it's not and we just want to close this and keep it as is!

References

This warning states that "In POSIX sh, 'local' is undefined".

`local` is used extensively in the mac script, but it should be
understood by most OSes based on the information in:
https://stackoverflow.com/questions/18597697/posix-compliant-way-to-scope-variables-to-a-function-in-a-shell-script#answer-18600920

Add a line at the beginning of the script to ignore this warning
in the entire file to avoid false positives when linting locally.
@rreinhardt9 rreinhardt9 marked this pull request as ready for review August 30, 2021 15:06
@cpytel cpytel deleted the branch thoughtbot:master March 30, 2022 16:49
@cpytel cpytel closed this Mar 30, 2022
@cpytel
Copy link
Member

cpytel commented Mar 30, 2022

@rreinhardt9 This PR was made against master, which no longer exists. I'd like to give you credit for the change, but you don't have your email listed publicly on GitHub. Feel free to email me at chad at thoughtbot.com and I can give you proper attribution for the commit. Thanks again.

@cpytel
Copy link
Member

cpytel commented Mar 30, 2022

New PR is here #606

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.

2 participants