-
Notifications
You must be signed in to change notification settings - Fork 33
Shellck5 #799
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
Shellck5 #799
Conversation
|
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look consistent throughout, LGTM
SMTP_USER="${SMTP_USER}" | ||
# shellcheck disable=SC2269 | ||
SMTP_PASSWORD="${SMTP_PASSWORD}" | ||
#required settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious what this change is here? Do we not actually need these lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We originally had lines assigning these variables the values already assigned to them (e.g. SMTP_HOST="${SMTP_HOST}"). Why? Mostly they were artifacts of the early implementation work, as we sorted out what parameters were needed..but the lines also clarified what inputs this functionality required or used. When shellck flagged them, I thought the latter reason was still valid and added the shellcheck disable
comments. But, later when looking over things one more time, I decided that keeping the line AND adding the disabling quotes was silly and the documentation objective could be achieved more cleanly with comments. Which is what we ended up with. I don't like the idea of code responding to apparently random environment variables. Documenting which environment variables are intended to influence behavior seems "better".
No description provided.