-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
small fixes to boilerplate and integration test scripts #3212
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: balopat The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
hack/boilerplate/fix.sh
Outdated
# limitations under the License. | ||
|
||
rootDir=$1 | ||
[[ "$rootDir" == "" ]] && export rootDir=`pwd` |
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.
Anything exported should be in all caps: ROOT_DIR
Also, here's a handy short cut for setting default shell values:
readonly ROOT_DIR=${$1:-$(pwd)}
export ROOT_DIR
hack/boilerplate/fix.sh
Outdated
[[ "$rootDir" == "" ]] && export rootDir=`pwd` | ||
|
||
function prepend() { | ||
ignore="vendor\|\_gopath\|assets.go" |
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.
Set these as local variables: https://google.github.io/styleguide/shell.xml?showone=Use_Local_Variables#Use_Local_Variables
hack/boilerplate/fix.sh
Outdated
pattern=$1 | ||
ref=$2 | ||
headers=$3 | ||
files=`hack/boilerplate/boilerplate.py --rootdir $rootDir | grep -v "$ignore" | grep "$pattern"` |
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.
Use $() instead of backticks: https://google.github.io/styleguide/shell.xml?showone=Command_Substitution#Command_Substitution
hack/boilerplate/fix.sh
Outdated
files=`hack/boilerplate/boilerplate.py --rootdir $rootDir | grep -v "$ignore" | grep "$pattern"` | ||
for f in $files; do | ||
echo $f; | ||
copyright="$(cat hack/boilerplate/boilerplate.$ref.txt | sed s/YEAR/2018/g)" |
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.
set copyright as local.
Move 2018 to a readonly constant.
hack/boilerplate/fix.sh
Outdated
echo $f; | ||
copyright="$(cat hack/boilerplate/boilerplate.$ref.txt | sed s/YEAR/2018/g)" | ||
|
||
if [ "$headers" != "" ]; then |
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.
[[ is preferred over [
https://google.github.io/styleguide/shell.xml?showone=Test,_%5B_and_%5B%5B#Test,_%5B_and_%5B%5B
Also: prefer
hack/boilerplate/fix.sh
Outdated
copyright="$(cat hack/boilerplate/boilerplate.$ref.txt | sed s/YEAR/2018/g)" | ||
|
||
if [ "$headers" != "" ]; then | ||
fileHeaders="$(cat $f | grep $headers)" |
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.
Use lowercase with underscores rather than camel case. Also, use "local".
hack/boilerplate/fix.sh
Outdated
|
||
if [ "$fileHeaders" != "" ]; then | ||
fileContent="$(cat $f | grep -v $headers)" | ||
printf '%s\n\n%s\n%s\n' "$fileHeaders" "$copyright" "$fileContent" > $f ; |
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.
No need for semicolon. Single space before >.
@tstromberg PTAL. |
No description provided.