Skip to content

Updated pre-commit shell script#655

Merged
bwendling merged 1 commit intogoogle:masterfrom
mmphego:master
Jan 14, 2019
Merged

Updated pre-commit shell script#655
bwendling merged 1 commit intogoogle:masterfrom
mmphego:master

Conversation

@mmphego
Copy link
Contributor

@mmphego mmphego commented Dec 5, 2018

  • Updated shebang from #!/bin/bash to #!/usr/bin/env bash for portability related issues.
  • Double quote to prevent globbing and word splitting.
  • Using command -v instead of which: The reason to select the former over the latter is that it avoids a dependency on something that is outside your shell.

Signed-off-by: Mpho Mphego mpho112@gmail.com

@coveralls
Copy link

coveralls commented Dec 8, 2018

Coverage Status

Coverage remained the same at 95.508% when pulling ff7a28d on mmphego:master into c810ead on google:master.

@mmphego
Copy link
Contributor Author

mmphego commented Dec 25, 2018

Ready to merge - Updated script.
@vapier

@mmphego
Copy link
Contributor Author

mmphego commented Jan 11, 2019

Ready to merge - Updated script.
@vapier

@bwendling
Copy link
Member

Could you squash all of the commits into one? It's hard to review them right now.

Updated shebang from `#!/bin/bash` to `#!/usr/bin/env bash` for portability(https://en.wikipedia.org/wiki/Shebang_%28Unix%29#Portability) related issues.
Double quote to prevent globbing and word splitting.
Using `command -v` instead of `which`: The reason to select the former over the latter is that it avoids a dependency on something that is outside your shell.

Signed-off-by: Mpho Mphego <mpho112@gmail.com>
@mmphego
Copy link
Contributor Author

mmphego commented Jan 14, 2019

Could you squash all of the commits into one? It's hard to review them right now.

My apologies - I have since updated the PR.

@bwendling
Copy link
Member

Thanks!

@bwendling bwendling merged commit 9546d40 into google:master Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants